Improve logging in boot.go to facilitate future triaging#38342
Improve logging in boot.go to facilitate future triaging#38342shunping wants to merge 4 commits intoapache:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the observability of the SDK container boot process by adding more granular logging. These changes facilitate easier debugging of environment-specific issues by providing clearer insights into pipeline configurations, experiment settings, and the state of Python dependencies before and after package installation. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request enhances logging within the Python container boot process by recording pipeline options, experiments, and runtime dependencies at both pre-installation and post-installation phases. It also updates the dependency logging to use pip freeze --all for better visibility. A security concern was identified regarding the logging of the entire PipelineOptions object, as it may inadvertently expose sensitive credentials such as API keys or access tokens.
|
r: @tvalentyn |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
| bufLogger.Printf(ctx, "Logging runtime dependencies:") | ||
| args = []string{"-m", "pip", "freeze"} | ||
| bufLogger.Printf(ctx, "Logging runtime dependencies (%s):", phase) | ||
| args = []string{"-m", "pip", "freeze", "--all"} |
There was a problem hiding this comment.
The argument "--all" ensures that the versions of pip, setuptools, etc are included in the result.
| bufLogger := tools.NewBufferedLogger(logger) | ||
| bufLogger.Printf(ctx, "Installing setup packages ...") | ||
|
|
||
| if err := logRuntimeDependencies(ctx, bufLogger, "pre-installation"); err != nil { |
There was a problem hiding this comment.
What is the use-case for this?
There was a problem hiding this comment.
While we already log dependencies after installation, boot.go exits immediately if any installation step fails.
Adding a pre-installation call ensures we capture the environment state regardless of whether the installation succeeds. This is useful for reproducing and triaging environment-specific failures from customers.
There was a problem hiding this comment.
'pre-installation', 'post-installation' sounds a bit cryptic for end user who is not a beam dev, and the output may be a a bit verbose.
How about we think of a way to enable debug logging for boot.go and only print pre-installation env if debug logging is enabled? then, we can ask affected customers to run their pipeline with debug logging enabled if necessary.
There was a problem hiding this comment.
The new logs only add 4 lines and are already emitted at the DEBUG level, which allows users to filter them out as needed.
Given this minimal footprint, I’d prefer to avoid adding complexity of a new configuration mechanism or flag in boot.go to keep the boot logic as simple as possible.
'pre-installation', 'post-installation' sounds a bit cryptic for end user who is not a beam dev.
I used the term "installation" because of the line of "Installing setup packages ..." prior to these logs (see above screenshot too), but I am open to any better term.
There was a problem hiding this comment.
i do think that the pre-installation output is confusing unless you know why you need to look at it; most of the time, you need to look at the final list after all the installations.
There was a problem hiding this comment.
Perhaps we could change logs to the below:
Installing setup packages -> Installing additional runtime dependencies if any are specified in --requirements_file, --setup_file or --extra_package options.
post-installation-> post-installation (final runtime environment)
There was a problem hiding this comment.
Great. I made the change to use "initial runtime environmemnt" and "final runtime environment". I also included some small edits to existing messages to make them consistent and concise. PTAL
We met some difficulty recently when triaging dataflow customer problems regarding Python environments.
Here, we add some more useful logging in the sdk image entrypoint
boot.go, which should help with the triaging process.Related internal bug: 491352862