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.
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
feat!(active_job): Use ActiveSupport instead of patches #677
feat!(active_job): Use ActiveSupport instead of patches #677
Changes from all commits
55e11e3
44cb141
9e20af7
76dd2a3
f710452
d3083b2
9042cdf
2ae1184
f79e1e8
918c919
92dad7a
0bad18c
dd297ef
b433aa7
e88c476
10021d1
45019ae
2b9078b
a345fa2
57e956e
8329bb5
6d2fbf8
3a56bdb
fe751b7
3adaf59
0eecaa5
6a6713f
d6bccf3
c7f84b8
8237302
0287169
c0655eb
50ac4b5
e3e096c
071dc7a
7639556
11799a9
7235421
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
I see a lot of duplication between this file and the Rack instrumentation file. It might be a good future candidate for a helper gem.
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!
I was of thinking of making a generic "ingress" span Context that would be instrumentation agnostic that way any child span is able to access the ingress span and enrich it.
So instead of saying
Rack.current_span
orActiveJob.current_span
it would beIngress.current_span
.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.
I think this is never called and that the context is never set.
This is causing link propagation to fail.
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.
Do you have any additional details you would like to share in an issue?
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.
Here we go! #1131
I was working on that, but it took me longer than expected to write up the issue. 😅
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.
This needs to be
::ActiveSupport::Notifications
, otherwise it throws: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.
Similarly, would you be able to provide us with a PR to address this problem?
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.
#1078