-
Notifications
You must be signed in to change notification settings - Fork 324
Context API beforeFinish Migration
#9422
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
Changes from 13 commits
796b57e
e8c2baf
737e783
c9a2e21
a0ecac7
2ef3dc7
a14848f
112ec20
909d174
c883e8d
b847401
bebe637
f76415f
8f4a6ee
762c233
c8c9ece
f715d62
7fe9c70
0f0f228
ecc5049
4bd005d
9b4d887
a3bb790
ee71b23
dd4c4c0
7fd52d8
2c27431
6e950a0
5f20aee
fd11e7a
0c4ab54
c13e8cd
b9ad08c
ce77411
b33528c
c62d570
841b27c
1f48c71
e89c230
e7d1a77
82884df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| import static datadog.trace.api.cache.RadixTreeCache.UNSET_PORT; | ||
| import static datadog.trace.bootstrap.instrumentation.java.net.HostNameResolver.hostName; | ||
|
|
||
| import datadog.context.Context; | ||
| import datadog.context.ContextScope; | ||
| import datadog.trace.api.Config; | ||
| import datadog.trace.api.DDTags; | ||
|
|
@@ -77,14 +78,18 @@ public AgentSpan afterStart(final AgentSpan span) { | |
| } | ||
|
|
||
| public AgentScope beforeFinish(final AgentScope scope) { | ||
| beforeFinish(scope.span()); | ||
| beforeFinish(scope.context()); | ||
| return scope; | ||
| } | ||
|
|
||
| public AgentSpan beforeFinish(final AgentSpan span) { | ||
| return span; | ||
| } | ||
|
Comment on lines
85
to
87
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❔ question: Can this one be removed then? It will prevent other decorator to override it and never being called.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cannot, WebSocketDecorator and its related instrumentation still call it. Needs to be a separate mini-migration
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that the only one blocker? Because the @amarziali What's the effort to migrate the WebSocket instrumentation to context rather than span?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another place where this is called is within the Mule Instrumentation as well via the Moved the empty call to I assume the actual span -> context functionality will be taken care of later for both WebSockets and Mule.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I take it back, I'm not sure how the tests aren't catching it but the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: It seems that there are no meaningful overrides of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update: The |
||
|
|
||
| public Context beforeFinish(final Context context) { | ||
| return context; | ||
| } | ||
|
|
||
| public AgentScope onError(final AgentScope scope, final Throwable throwable) { | ||
| onError(scope.span(), throwable); | ||
| return scope; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.