-
Notifications
You must be signed in to change notification settings - Fork 315
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
base: master
Are you sure you want to change the base?
Changes from all 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
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 |
|---|---|---|
|
|
@@ -56,7 +56,7 @@ public abstract class HttpServerDecorator<REQUEST, CONNECTION, RESPONSE, REQUEST | |
| private static final Logger log = LoggerFactory.getLogger(HttpServerDecorator.class); | ||
| private static final int UNSET_PORT = 0; | ||
|
|
||
| public static final String DD_SPAN_ATTRIBUTE = "datadog.span"; | ||
| public static final String DD_CONTEXT_ATTRIBUTE = "datadog.context"; | ||
| public static final String DD_DISPATCH_SPAN_ATTRIBUTE = "datadog.span.dispatch"; | ||
| public static final String DD_RUM_INJECTED = "datadog.rum.injected"; | ||
| public static final String DD_FIN_DISP_LIST_SPAN_ATTRIBUTE = | ||
|
|
@@ -537,12 +537,14 @@ private Flow<Void> callIGCallbackURI( | |
| } | ||
|
|
||
| @Override | ||
| public AgentSpan beforeFinish(AgentSpan span) { | ||
| // TODO Migrate beforeFinish to Context API | ||
|
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: For cross reference, it's worth mentioning #9388 in this PR description. |
||
| public Context beforeFinish(Context context) { | ||
| AgentSpan span = AgentSpan.fromContext(context); | ||
zarirhamza marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| onRequestEndForInstrumentationGateway(span); | ||
|
|
||
| // Close Serverless Gateway Inferred Span if any | ||
| // finishInferredProxySpan(context); | ||
|
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. @zarirhamza This PR is more than just a refactoring. It now calls |
||
| return super.beforeFinish(span); | ||
| finishInferredProxySpan(context); | ||
zarirhamza marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| return super.beforeFinish(context); | ||
| } | ||
|
|
||
| protected void finishInferredProxySpan(Context context) { | ||
|
|
||
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.
❔ question: Can this one be removed then? It will prevent other decorator to override it and never being called.
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.
Cannot, WebSocketDecorator and its related instrumentation still call it. Needs to be a separate mini-migration
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.
Is that the only one blocker? Because the
WebSocketDecoratoris calling the emptyBaseDecorator.beforeFinish()and has no override... So technically, the call is doing nothing :/So we can move it from
BaseDecoratortoWebSocketDecoratorand migrate it in a second pass.@amarziali What's the effort to migrate the WebSocket instrumentation to context rather than 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.
Another place where this is called is within the Mule Instrumentation as well via the
MuleDecoratorin mule-4.Moved the empty call to
WebSocketDecoratorandMuleDecoratorthus allowing me to remove the function fromBaseDecorator.I assume the actual span -> context functionality will be taken care of later for both WebSockets and Mule.
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 take it back, I'm not sure how the tests aren't catching it but the
ClientDecoratoralso usesbeforeFinish(span)via theBaseDecorator. This seems to indicate that we need to invest time into looking through all decorators and confirming their reliance on beforeFinish. To save time, for now we can focus just onHttpServerDecoratorrelated instrumentationUh oh!
There was an error while loading. Please reload this page.
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.
Suggestion: It seems that there are no meaningful overrides of
beforeFinish(final AgentSpan span), except forAxisMessageDecorator, which takes an extra parameter. To avoid confusion, let's go ahead and removebeforeFinish(final AgentSpan span).