Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(#25314) Have boot loaders log to logging service where possible. #26035

Merged
merged 7 commits into from
Mar 31, 2023

Conversation

lostluck
Copy link
Contributor

@lostluck lostluck commented Mar 29, 2023

Have the boot loaders log to the logging service once we have an endpoint. This allows their severity to be recorded properly for better surfacing in runners, such as for fatal errors.

Sets the default log level to Debug for boot loader logging instead of Info, since boot loader configuration is unlikely to be of interest to most Beam users. Previously fatal logs use.

Also moves the "provision" code to a package path to indicate it's for Beam's container bootloader use, and not for Beam Go SDK users. It was never intended for end users, so this reduction in surface clarifies the intention among Go SDK packages.

Fixes #25314 and #24470 which were both around the incorrect surfacing of these errors as "info", and how they hampered debugging.

This also serves as a stopgap for issues in #25582 until that's resolved.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI.

@lostluck
Copy link
Contributor Author

Since this is changing the boot loaders, the test set is rather large.

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #26035 (120f484) into master (5bdc452) will increase coverage by 0.09%.
The diff coverage is 21.42%.

@@            Coverage Diff             @@
##           master   #26035      +/-   ##
==========================================
+ Coverage   71.34%   71.43%   +0.09%     
==========================================
  Files         779      782       +3     
  Lines      102767   102847      +80     
==========================================
+ Hits        73319    73471     +152     
+ Misses      27976    27900      -76     
- Partials     1472     1476       +4     
Flag Coverage Δ
go 54.09% <21.42%> (+0.30%) ⬆️
python 79.97% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sdks/go/pkg/beam/core/runtime/harness/datamgr.go 56.61% <ø> (-0.47%) ⬇️
sdks/go/pkg/beam/core/runtime/harness/harness.go 11.11% <0.00%> (+0.08%) ⬆️
sdks/go/pkg/beam/core/runtime/harness/logging.go 46.08% <ø> (+20.12%) ⬆️
sdks/go/pkg/beam/provision/provision.go 60.00% <60.00%> (+12.50%) ⬆️

... and 12 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link
Contributor

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @pabloem for label python.
R: @kennknowles for label java.
R: @jrmccluskey for label go.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@jrmccluskey
Copy link
Contributor

Are we okay to delete the provision package in pkg/beam with this change?

@lostluck
Copy link
Contributor Author

Are we okay to delete the provision package in pkg/beam with this change?

I'm of the opinion it shouldn't be there, as it promotes arbitrary use of implementation details, and makes an already difficult to learn system, harder.

I would concede to leave in vestigial functions that call the new location for the functions.

But it seems unlikely that there are more than the 0 public users of the package who would benefit from the migration.

@lostluck
Copy link
Contributor Author

lostluck commented Mar 30, 2023

Hmmm, there are some uses internal to Dataflow. I'll add things back so those can be more gracefully migrated.

Edit: OK, I swore I had deleted the package in the migration, but apparently that change didn't stick. Going to do the call forward thing for now. Also, I missed the typescript container. No SDK left behind!

Copy link
Contributor

@jrmccluskey jrmccluskey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the refactoring of the provision package within pkg/beam + deprecation warnings, I'm pretty content with this change. Thank you!

@lostluck
Copy link
Contributor Author

R: @robertwb for typescript and general experience with the bootloaders commentary.

@github-actions
Copy link
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

@lostluck
Copy link
Contributor Author

@robertwb has confirmed offline that the typescript tests can be ignored for now (they don't push their own build/dev containers WRT testing PRs.) Ah, the trials of an experimental SDK.

@lostluck
Copy link
Contributor Author

The go test action failure was dominikh/go-tools#1034 which appears to be fixed in more recent staticheck versions (currently on 2022.1). I'll move us to a more recent version.

This seems to have resolved it too. Actions also now use Go 1.20+

@lostluck
Copy link
Contributor Author

Looks like github API quota was hit, and that killed a few of the jenkins jobs.

@lostluck
Copy link
Contributor Author

Run GoPortable PreCommit

@lostluck
Copy link
Contributor Author

Run Java PreCommit

@lostluck
Copy link
Contributor Author

Run Java_Examples_Dataflow PreCommit

@lostluck
Copy link
Contributor Author

Run Portable_Python PreCommit

@lostluck
Copy link
Contributor Author

Run PythonDocker PreCommit

@lostluck
Copy link
Contributor Author

Run Python_Dataframes PreCommit

@lostluck
Copy link
Contributor Author

Run Python_PVR_Flink PreCommit

@lostluck
Copy link
Contributor Author

Run Python_Transforms PreCommit

@lostluck
Copy link
Contributor Author

Run Spotless PreCommit

@lostluck
Copy link
Contributor Author

Failures and pending are the CodeCov (can't help it right now, due to the boot loaders being untested), and the non-blocking typescript tests. Merging!

@lostluck lostluck merged commit 492e2c9 into master Mar 31, 2023
@kennknowles
Copy link
Member

Nice. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: SDK process termination bypasses Fn Logging API
3 participants