Adding context.Context hub extractor for sentry-logrus integration #522
Conversation
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
…s set for logrus.Entry
|
Hello, @cleptric. Any comments for this? Do I need to resolve typo, or smth else? |
|
@090809 Thanks for the contribution. We are currently busy with other things but will take a look as soon as we wrap up. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Ho-ho-ho!
Problem:
When using sentry-logrus integration, it's no option for capturing events with connected traces, 'cause scopes between global sentry and hook integration isn't same.
Probably solution:
There no way to "just add
hook.NewFromHub(levels, hub)", -> it will race condition.Solution:
Adding custom extraction for sentry.Hub from current context (with using
logrus.WithContext(ctx)and injectedsentry.Hubto ctx viaSetHubOnContext, or any middlewares, or smth else) solves this clearly.Current change doesn't broke current API, but it gives ability for resolving explained problem.
P.S.:
Probably, test isn't clearly, but I not found any more clear solution for test this, whithout more API changing