-
Notifications
You must be signed in to change notification settings - Fork 505
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
Global Log handler cleanup - Logs SDK #2184
Merged
lalitb
merged 20 commits into
open-telemetry:main
from
lalitb:global-error-handler-cleanup-logs-sdk
Oct 14, 2024
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
d7fc754
initial commit
lalitb f6e5cb9
Merge branch 'main' into global-error-handler-cleanup-logs-sdk
lalitb 76bafef
comments
lalitb 3006c16
Merge branch 'global-error-handler-cleanup-logs-sdk' of github.com:la…
lalitb d63957d
Merge branch 'main' into global-error-handler-cleanup-logs-sdk
lalitb 96e652a
make SendResultError as warning
lalitb a4a812f
Merge branch 'global-error-handler-cleanup-logs-sdk' of github.com:la…
lalitb 886a583
simplify internal logs formatting
lalitb 0d5bf14
change non-actionable error to debug
lalitb a8d772e
Merge branch 'main' into global-error-handler-cleanup-logs-sdk
lalitb c25923d
review comments
lalitb b8819c4
Merge branch 'global-error-handler-cleanup-logs-sdk' of github.com:la…
lalitb 0bd341b
cont..
lalitb 4979249
lint
lalitb f1182ff
handle loggerprovider_drop
lalitb 40f7a5c
add todo to prevert flooding
lalitb f695605
Merge branch 'main' into global-error-handler-cleanup-logs-sdk
lalitb e99195b
Merge branch 'main' into global-error-handler-cleanup-logs-sdk
lalitb a566aac
Merge branch 'main' into global-error-handler-cleanup-logs-sdk
cijothomas 8d88392
fix merge errors
lalitb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this be triggered when channel is full? If yes, we need to rethink this, as this can spam the log output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this error only triggers when channel is full or closed. We need to add some throttling or logic to prevent flooding - have added the TODO for now, as we need common strategy for such flooding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree we need a common strategy, but lets remove the error log from here. It'll flood as-is when buffer is full.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we remove altogether, or make it otel_debug for now - with comment to change it to otel_error once throttling is ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either of them are fine with me, though I slightly prefer removing altogether, as I don't know if we can ship a throttling solution for next release.