flush traces on shutdown #2882
flush traces on shutdown #2882
Conversation
WalkthroughThe recent updates to the core/metrics package enhance the Changes
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 Configuration File (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
- Added
unwrappedTPfield tobaseHandlerin/core/metrics/base.gofor trace flushing on shutdown - Modified
newBaseHandlerandnewBaseHandlerWithTracerProviderin/core/metrics/base.goto handleunwrappedTP - Introduced
handleShutdownfunction in/core/metrics/otlp.gofor graceful shutdown of traces - Updated
Startmethod in/core/metrics/otlp.goto callForceFlushandShutdownon context cancellation
2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
|
|
||
| // allow only 10 seconds for graceful shutdown. | ||
| // we use without cancel to copy the parents values while making sure are derived context is not canceled. | ||
| shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), shutdownAllowance) |
There was a problem hiding this comment.
Spelling: Typo: 'are' should be 'our'.
| shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), shutdownAllowance) | |
| // we use without cancel to copy the parents values while making sure our derived context is not canceled. |
Deploying sanguine-fe with
|
| Latest commit: |
1397f8f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://094f9c8f.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-trace-flush.sanguine-fe.pages.dev |
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced graceful shutdown mechanism in
/core/metrics/otlp.go - Ensured traces are flushed within a 10-second allowance during shutdown
- Created derived context with timeout for shutdown process
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
core/metrics/otlp.go (1)
66-86: Review ofhandleShutdownfunction.
- Context Handling: The function waits for the context to be canceled before proceeding with the shutdown, which is a good practice for graceful termination.
- Shutdown Timeout: A 10-second timeout is enforced for the shutdown process. This is a reasonable choice to balance between a quick shutdown and giving enough time for operations to complete.
- Error Handling: Errors during the flush and shutdown processes are logged but not escalated. This might be acceptable depending on the application's error handling strategy, but it would be wise to consider if there should be further action taken on these errors.
- Use of
context.WithoutCancel: This is an interesting choice. It ensures that the derived context inherits the values from the parent context without being affected by its cancellation. This is crucial in ensuring that the shutdown process can complete even if the parent context is canceled.Overall, the function is well-implemented with proper use of contexts and error handling. However, consider the implications of not escalating errors during the shutdown process.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- core/metrics/base.go (3 hunks)
- core/metrics/otlp.go (2 hunks)
Additional comments not posted (2)
core/metrics/otlp.go (1)
55-58: Initiate graceful shutdown in a separate goroutine.The implementation correctly starts a separate goroutine to handle the shutdown process. This ensures that the main execution flow is not blocked.
core/metrics/base.go (1)
40-47: Structural changes inbaseHandler.The
baseHandlerstruct now includes theunwrappedTPfield, which is used only for shutdown. This is a good separation of concerns, allowing the regular tracer provider to be unaffected by shutdown operations. The reordering of fields and the removal ofresourceare significant changes that should be verified across the codebase to ensure no side effects.Verification successful
Verify structural changes in
baseHandler.The structural changes in
baseHandler, including the addition of theunwrappedTPfield, appear to be consistently integrated across the codebase. TheunwrappedTPfield is used appropriately for shutdown operations, and the instantiation ofbaseHandleraligns with the new structure.
core/metrics/otlp.gocore/metrics/tester.gocore/metrics/jaeger.gocore/metrics/base.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `baseHandler` across the codebase to ensure no side effects from the structural changes. # Test: Search for the usage of `baseHandler`. Expect: Consistent usage considering the new structure. rg --type go -A 5 $'baseHandler'Length of output: 8217
| unwrappedTP := tracesdk.NewTracerProvider(opts...) | ||
| tp := PyroscopeWrapTracerProvider(unwrappedTP, buildInfo) | ||
| // will do nothing if not enabled. | ||
| StartPyroscope(buildInfo) | ||
|
|
||
| propagator := b3.New(b3.WithInjectEncoding(b3.B3MultipleHeader | b3.B3SingleHeader)) | ||
| return newBaseHandlerWithTracerProvider(rsr, buildInfo, tp, propagator) | ||
| return newBaseHandlerWithTracerProvider(rsr, buildInfo, unwrappedTP, tp, propagator) |
There was a problem hiding this comment.
Review of newBaseHandler and newBaseHandlerWithTracerProvider.
- Function Signature Changes: The addition of
unwrappedTPas a parameter before other tracer-related parameters helps in segregating the shutdown-specific tracer provider from others. This is a strategic design choice to enhance modularity and clarity. - Implementation Details: The function setups are quite complex, involving multiple OpenTelemetry configurations and custom logger setups. It's crucial that these configurations are thoroughly tested, especially since they involve external dependencies and environmental configurations.
- Error Handling: There is a TODO comment about handling errors which should be addressed to ensure robustness.
These changes are well thought out but require thorough testing and error handling strategies to be fully effective.
Would you like me to help with implementing the error handling or testing strategies mentioned in the TODO comments?
Also applies to: 243-243
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2882 +/- ##
===================================================
+ Coverage 23.56910% 23.95059% +0.38148%
===================================================
Files 679 628 -51
Lines 50842 48408 -2434
Branches 80 80
===================================================
- Hits 11983 11594 -389
+ Misses 37536 35664 -1872
+ Partials 1323 1150 -173
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced graceful shutdown mechanism in
/core/metrics/otlp.go - Ensured traces are flushed within a 10-second allowance during shutdown
- Created derived context with timeout for shutdown process
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- core/metrics/otlp.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- core/metrics/otlp.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- core/metrics/otlp.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- core/metrics/otlp.go
Description
Flushes all traces on shutdown
Summary by CodeRabbit