fix: do not use hardcoded AlwaysSample option
#61
Conversation
Signed-off-by: Valery Piashchynski <[email protected]>
WalkthroughThis update to the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- plugin.go (2 hunks)
Additional comments not posted (3)
plugin.go (3)
140-148: Ensure proper error handling during tracer shutdown.Verification successful
The review comment concerns the error handling during the tracer shutdown in the
plugin.gofile. The script output indicates that thetracer.Shutdownmethod is called only once in this file, and there are no other instances in the codebase to compare against for consistency.Given this context, let's examine the provided code snippet more closely:
func (p *Plugin) Stop(ctx context.Context) error { // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#forceflush err := p.tracer.ForceFlush(ctx) if err != nil { return err } // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#shutdown err = p.tracer.Shutdown(ctx)From the snippet, it appears that the error handling for
tracer.Shutdownis not completed, as the method call is shown but not the subsequent error check. This might be an oversight or an incomplete code snippet.To verify whether the error from
tracer.Shutdownis handled properly, we need to see the lines of code following thetracer.Shutdowncall.
The review comment about ensuring proper error handling during tracer shutdown in
plugin.gohas been addressed in the code. The error from thetracer.Shutdownmethod is appropriately checked and handled:err = p.tracer.Shutdown(ctx) if err != nil { return err } return nilThis pattern is consistent with Go's best practices for error handling, where errors are handled immediately after the operation that might generate them. Therefore, the error handling is robust and follows the expected standards.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the error handling during tracer shutdown is robust and follows best practices. # Test: Search for similar error handling patterns in other parts of the codebase. Expect: Consistent and robust error handling. rg --type go $'tracer.Shutdown'Length of output: 70
Script:
#!/bin/bash # Description: Fetch the lines following the tracer.Shutdown call to verify error handling. # Using `rg` to find the line number and `sed` to extract the subsequent lines. rg --type go -n 'tracer.Shutdown' plugin.go | sed -n -e 's/^\([0-9]*\):.*/\1/p' | xargs -I {} sed -n '{},$p' plugin.goLength of output: 2143
140-148: Initialization now correctly uses configuration-driven settings.
140-148: Graceful shutdown of tracer is handled correctly.
Reason for This PR
closes: roadrunner-server/roadrunner#1918
Description of Changes
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit