Context API beforeFinish Migration
#9422
Open
+1,093
−655
Conversation
This file contains hidden or 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
* Creates inferred proxy spans for API Gateway calls via presence of http headers --------- Co-authored-by: Zarir Hamza <[email protected]>
Avoid duplicate expensive context extraction Avoid subclassing tracing span for serverless but used serverless context element instead to store / track inferred span while keep tracing feature untouched Improved propagator to not create / capture inferred span context element on invalid data Rework context element to hold the inferred spans and its captured data Release captured data as soon as they span start (never read after this point so reclaiming memory) Refactor context element and propagator into the right package, not context component (product / feature agnostic) Refactor unit tests
zarirhamza
added
inst: others
|
🎯 Code Coverage 🔗 Commit SHA: 9b4d887 | Docs | Was this helpful? Give us feedback! |
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 46 metrics, 13 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.53.0-SNAPSHOT~9b4d887cb8, baseline=1.53.0-SNAPSHOT~d07fee98fb
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.062 s) : 0, 1061802
Total [baseline] (10.906 s) : 0, 10906257
Agent [candidate] (1.051 s) : 0, 1050955
Total [candidate] (10.738 s) : 0, 10738089
section appsec
Agent [baseline] (1.224 s) : 0, 1223960
Total [baseline] (10.815 s) : 0, 10814564
Agent [candidate] (1.223 s) : 0, 1223278
Total [candidate] (10.775 s) : 0, 10774515
section iast
Agent [baseline] (1.183 s) : 0, 1182771
Total [baseline] (10.931 s) : 0, 10930790
Agent [candidate] (1.186 s) : 0, 1186039
Total [candidate] (10.981 s) : 0, 10980681
section profiling
Agent [baseline] (1.198 s) : 0, 1198305
Total [baseline] (10.907 s) : 0, 10907304
Agent [candidate] (1.198 s) : 0, 1197573
Total [candidate] (10.888 s) : 0, 10887543
gantt
title petclinic - break down per module: candidate=1.53.0-SNAPSHOT~9b4d887cb8, baseline=1.53.0-SNAPSHOT~d07fee98fb
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.472 ms) : 0, 1472
crashtracking [candidate] (1.456 ms) : 0, 1456
BytebuddyAgent [baseline] (744.194 ms) : 0, 744194
BytebuddyAgent [candidate] (734.596 ms) : 0, 734596
GlobalTracer [baseline] (245.049 ms) : 0, 245049
GlobalTracer [candidate] (243.19 ms) : 0, 243190
AppSec [baseline] (30.547 ms) : 0, 30547
AppSec [candidate] (30.065 ms) : 0, 30065
Debugger [baseline] (6.147 ms) : 0, 6147
Debugger [candidate] (6.045 ms) : 0, 6045
Remote Config [baseline] (680.428 µs) : 0, 680
Remote Config [candidate] (674.887 µs) : 0, 675
Telemetry [baseline] (12.347 ms) : 0, 12347
Telemetry [candidate] (13.78 ms) : 0, 13780
section appsec
crashtracking [baseline] (1.452 ms) : 0, 1452
crashtracking [candidate] (1.446 ms) : 0, 1446
BytebuddyAgent [baseline] (755.956 ms) : 0, 755956
BytebuddyAgent [candidate] (755.42 ms) : 0, 755420
GlobalTracer [baseline] (235.641 ms) : 0, 235641
GlobalTracer [candidate] (235.58 ms) : 0, 235580
IAST [baseline] (23.508 ms) : 0, 23508
IAST [candidate] (23.494 ms) : 0, 23494
AppSec [baseline] (171.391 ms) : 0, 171391
AppSec [candidate] (169.806 ms) : 0, 169806
Debugger [baseline] (5.782 ms) : 0, 5782
Debugger [candidate] (7.297 ms) : 0, 7297
Remote Config [baseline] (638.338 µs) : 0, 638
Remote Config [candidate] (646.226 µs) : 0, 646
Telemetry [baseline] (8.427 ms) : 0, 8427
Telemetry [candidate] (8.434 ms) : 0, 8434
section iast
crashtracking [baseline] (1.458 ms) : 0, 1458
crashtracking [candidate] (1.458 ms) : 0, 1458
BytebuddyAgent [baseline] (853.721 ms) : 0, 853721
BytebuddyAgent [candidate] (855.167 ms) : 0, 855167
GlobalTracer [baseline] (233.836 ms) : 0, 233836
GlobalTracer [candidate] (235.129 ms) : 0, 235129
IAST [baseline] (31.122 ms) : 0, 31122
IAST [candidate] (30.515 ms) : 0, 30515
AppSec [baseline] (26.823 ms) : 0, 26823
AppSec [candidate] (27.852 ms) : 0, 27852
Debugger [baseline] (5.747 ms) : 0, 5747
Debugger [candidate] (5.821 ms) : 0, 5821
Remote Config [baseline] (595.118 µs) : 0, 595
Remote Config [candidate] (609.583 µs) : 0, 610
Telemetry [baseline] (8.317 ms) : 0, 8317
Telemetry [candidate] (8.394 ms) : 0, 8394
section profiling
crashtracking [baseline] (1.446 ms) : 0, 1446
crashtracking [candidate] (1.46 ms) : 0, 1460
BytebuddyAgent [baseline] (763.33 ms) : 0, 763330
BytebuddyAgent [candidate] (762.327 ms) : 0, 762327
GlobalTracer [baseline] (222.374 ms) : 0, 222374
GlobalTracer [candidate] (222.848 ms) : 0, 222848
AppSec [baseline] (30.107 ms) : 0, 30107
AppSec [candidate] (30.064 ms) : 0, 30064
Debugger [baseline] (6.316 ms) : 0, 6316
Debugger [candidate] (7.008 ms) : 0, 7008
Remote Config [baseline] (703.832 µs) : 0, 704
Remote Config [candidate] (717.247 µs) : 0, 717
Telemetry [baseline] (16.387 ms) : 0, 16387
Telemetry [candidate] (15.491 ms) : 0, 15491
ProfilingAgent [baseline] (107.891 ms) : 0, 107891
ProfilingAgent [candidate] (107.875 ms) : 0, 107875
Profiling [baseline] (108.545 ms) : 0, 108545
Profiling [candidate] (108.535 ms) : 0, 108535
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.53.0-SNAPSHOT~9b4d887cb8, baseline=1.53.0-SNAPSHOT~d07fee98fb
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.054 s) : 0, 1053928
Total [baseline] (8.654 s) : 0, 8653739
Agent [candidate] (1.049 s) : 0, 1048845
Total [candidate] (8.609 s) : 0, 8608546
section iast
Agent [baseline] (1.187 s) : 0, 1187209
Total [baseline] (9.378 s) : 0, 9377623
Agent [candidate] (1.18 s) : 0, 1180023
Total [candidate] (9.41 s) : 0, 9409893
gantt
title insecure-bank - break down per module: candidate=1.53.0-SNAPSHOT~9b4d887cb8, baseline=1.53.0-SNAPSHOT~d07fee98fb
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.463 ms) : 0, 1463
crashtracking [candidate] (1.452 ms) : 0, 1452
BytebuddyAgent [baseline] (738.478 ms) : 0, 738478
BytebuddyAgent [candidate] (733.745 ms) : 0, 733745
GlobalTracer [baseline] (244.62 ms) : 0, 244620
GlobalTracer [candidate] (243.309 ms) : 0, 243309
AppSec [baseline] (30.413 ms) : 0, 30413
AppSec [candidate] (30.197 ms) : 0, 30197
Debugger [baseline] (6.154 ms) : 0, 6154
Debugger [candidate] (6.098 ms) : 0, 6098
Remote Config [baseline] (689.412 µs) : 0, 689
Remote Config [candidate] (677.682 µs) : 0, 678
Telemetry [baseline] (10.929 ms) : 0, 10929
Telemetry [candidate] (12.345 ms) : 0, 12345
section iast
crashtracking [baseline] (1.458 ms) : 0, 1458
crashtracking [candidate] (1.452 ms) : 0, 1452
BytebuddyAgent [baseline] (857.594 ms) : 0, 857594
BytebuddyAgent [candidate] (851.021 ms) : 0, 851021
GlobalTracer [baseline] (235.051 ms) : 0, 235051
GlobalTracer [candidate] (233.98 ms) : 0, 233980
IAST [baseline] (28.025 ms) : 0, 28025
IAST [candidate] (27.984 ms) : 0, 27984
AppSec [baseline] (29.289 ms) : 0, 29289
AppSec [candidate] (28.829 ms) : 0, 28829
Debugger [baseline] (5.736 ms) : 0, 5736
Debugger [candidate] (6.678 ms) : 0, 6678
Remote Config [baseline] (625.246 µs) : 0, 625
Remote Config [candidate] (604.483 µs) : 0, 604
Telemetry [baseline] (8.309 ms) : 0, 8309
Telemetry [candidate] (8.399 ms) : 0, 8399
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 2 performance regressions! Performance is the same for 10 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.53.0-SNAPSHOT~9b4d887cb8, baseline=1.53.0-SNAPSHOT~d07fee98fb
dateFormat X
axisFormat %s
section baseline
no_agent (36.486 ms) : 36189, 36783
. : milestone, 36486,
appsec (47.53 ms) : 47098, 47963
. : milestone, 47530,
code_origins (46.081 ms) : 45680, 46482
. : milestone, 46081,
iast (44.316 ms) : 43951, 44681
. : milestone, 44316,
profiling (46.475 ms) : 46076, 46874
. : milestone, 46475,
tracing (44.706 ms) : 44333, 45080
. : milestone, 44706,
section candidate
no_agent (37.146 ms) : 36847, 37444
. : milestone, 37146,
appsec (47.658 ms) : 47216, 48101
. : milestone, 47658,
code_origins (44.793 ms) : 44397, 45188
. : milestone, 44793,
iast (44.446 ms) : 44050, 44841
. : milestone, 44446,
profiling (50.347 ms) : 49897, 50796
. : milestone, 50347,
tracing (46.372 ms) : 45967, 46778
. : milestone, 46372,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.53.0-SNAPSHOT~9b4d887cb8, baseline=1.53.0-SNAPSHOT~d07fee98fb
dateFormat X
axisFormat %s
section baseline
no_agent (4.379 ms) : 4324, 4434
. : milestone, 4379,
iast (9.204 ms) : 9044, 9364
. : milestone, 9204,
iast_FULL (13.686 ms) : 13420, 13952
. : milestone, 13686,
iast_GLOBAL (10.346 ms) : 10153, 10538
. : milestone, 10346,
profiling (8.913 ms) : 8773, 9054
. : milestone, 8913,
tracing (7.451 ms) : 7346, 7556
. : milestone, 7451,
section candidate
no_agent (4.381 ms) : 4325, 4438
. : milestone, 4381,
iast (9.085 ms) : 8935, 9235
. : milestone, 9085,
iast_FULL (14.153 ms) : 13876, 14430
. : milestone, 14153,
iast_GLOBAL (10.221 ms) : 10041, 10401
. : milestone, 10221,
profiling (8.659 ms) : 8510, 8808
. : milestone, 8659,
tracing (7.536 ms) : 7419, 7653
. : milestone, 7536,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 1 unstable metrics. Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.53.0-SNAPSHOT~9b4d887cb8, baseline=1.53.0-SNAPSHOT~d07fee98fb
dateFormat X
axisFormat %s
section baseline
no_agent (15.075 s) : 15075000, 15075000
. : milestone, 15075000,
appsec (15.01 s) : 15010000, 15010000
. : milestone, 15010000,
iast (18.374 s) : 18374000, 18374000
. : milestone, 18374000,
iast_GLOBAL (18.137 s) : 18137000, 18137000
. : milestone, 18137000,
profiling (15.033 s) : 15033000, 15033000
. : milestone, 15033000,
tracing (14.98 s) : 14980000, 14980000
. : milestone, 14980000,
section candidate
no_agent (15.251 s) : 15251000, 15251000
. : milestone, 15251000,
appsec (15.145 s) : 15145000, 15145000
. : milestone, 15145000,
iast (18.38 s) : 18380000, 18380000
. : milestone, 18380000,
iast_GLOBAL (18.234 s) : 18234000, 18234000
. : milestone, 18234000,
profiling (15.397 s) : 15397000, 15397000
. : milestone, 15397000,
tracing (15.069 s) : 15069000, 15069000
. : milestone, 15069000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.53.0-SNAPSHOT~9b4d887cb8, baseline=1.53.0-SNAPSHOT~d07fee98fb
dateFormat X
axisFormat %s
section baseline
no_agent (1.474 ms) : 1462, 1485
. : milestone, 1474,
appsec (3.66 ms) : 3444, 3877
. : milestone, 3660,
iast (2.191 ms) : 2128, 2255
. : milestone, 2191,
iast_GLOBAL (2.252 ms) : 2189, 2316
. : milestone, 2252,
profiling (2.045 ms) : 1994, 2095
. : milestone, 2045,
tracing (2.017 ms) : 1968, 2067
. : milestone, 2017,
section candidate
no_agent (1.473 ms) : 1461, 1484
. : milestone, 1473,
appsec (3.654 ms) : 3437, 3871
. : milestone, 3654,
iast (2.2 ms) : 2137, 2264
. : milestone, 2200,
iast_GLOBAL (2.255 ms) : 2190, 2320
. : milestone, 2255,
profiling (2.069 ms) : 2016, 2122
. : milestone, 2069,
tracing (2.018 ms) : 1969, 2068
. : milestone, 2018,
|
zarirhamza
requested review from
robertpi,
sezen-datadog,
dougqh and
PerfectSlayer
and removed request for
a team
zarirhamza
changed the title
beforeFinish Migration
There was a problem hiding this comment.
I only had a quick review so far but it looks promising 👍
We need to #9384 review an merged to prevent conflicts. Can you target this branch instead?
Can you also remove what's already part of #9388? It will get better to review if we keep this PR related to the beforeFinish only. I will target mine onto yours then.
Comment on lines
85
to
87
| public AgentSpan beforeFinish(final AgentSpan span) { | ||
| return span; | ||
| } |
There was a problem hiding this comment.
❔ 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.
Cannot, WebSocketDecorator and its related instrumentation still call it. Needs to be a separate mini-migration
There was a problem hiding this comment.
Is that the only one blocker? Because the WebSocketDecorator is calling the empty BaseDecorator.beforeFinish() and has no override... So technically, the call is doing nothing :/
So we can move it from BaseDecorator to WebSocketDecorator and 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.
Another place where this is called is within the Mule Instrumentation as well via the MuleDecorator in mule-4.
Moved the empty call to WebSocketDecorator and MuleDecorator thus allowing me to remove the function from BaseDecorator.
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.
I take it back, I'm not sure how the tests aren't catching it but the ClientDecorator also uses beforeFinish(span) via the BaseDecorator. 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 on HttpServerDecorator related instrumentation
...kka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/DatadogWrapperHelper.java
Outdated
Show resolved
Hide resolved
...kka-http-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/DatadogWrapperHelper.java
Outdated
Show resolved
Hide resolved
...2/src/main/java/datadog/trace/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java
Show resolved
Hide resolved
...http-2.3.20/src/main/java/datadog/trace/instrumentation/grizzlyhttp232/GrizzlyDecorator.java
Show resolved
Hide resolved
...dertow-2.0/src/main/java/datadog/trace/instrumentation/undertow/ExchangeEndSpanListener.java
Outdated
Show resolved
Hide resolved
...bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/BaseDecorator.java
Outdated
Show resolved
Hide resolved
zarirhamza
changed the base branch from
master
to
bbujon/gateway-inferred-span2
zarirhamza
changed the base branch from
bbujon/gateway-inferred-span2
to
master
Comment on lines
85
to
87
| public AgentSpan beforeFinish(final AgentSpan span) { | ||
| return span; | ||
| } |
There was a problem hiding this comment.
Is that the only one blocker? Because the WebSocketDecorator is calling the empty BaseDecorator.beforeFinish() and has no override... So technically, the call is doing nothing :/
So we can move it from BaseDecorator to WebSocketDecorator and migrate it in a second pass.
@amarziali What's the effort to migrate the WebSocket instrumentation to context rather than span?
...entation/cxf-2.1/src/main/java/datadog/trace/instrumentation/cxf/InvokerInstrumentation.java
Outdated
Show resolved
Hide resolved
...ain/java/datadog/trace/instrumentation/grizzlyhttp232/DefaultFilterChainInstrumentation.java
Outdated
Show resolved
Hide resolved
...zly-http-2.3.20/src/main/java/datadog/trace/instrumentation/grizzlyhttp232/FilterAdvice.java
Outdated
Show resolved
Hide resolved
...zly-http-2.3.20/src/main/java/datadog/trace/instrumentation/grizzlyhttp232/FilterAdvice.java
Outdated
Show resolved
Hide resolved
...http-2.3.20/src/main/java/datadog/trace/instrumentation/grizzlyhttp232/GrizzlyDecorator.java
Outdated
Show resolved
Hide resolved
...t-1.1/src/main/java/datadog/trace/instrumentation/jaxrs/v1/JaxRsClientV1Instrumentation.java
Outdated
Show resolved
Hide resolved
| @@ -53,42 +55,58 @@ public void methodAdvice(MethodTransformer transformer) { | |||
|
|
|||
| static class HandleAdvice { | |||
| @Advice.OnMethodEnter(suppress = Throwable.class) | |||
| static AgentScope onEnter( | |||
| static ContextScope onEnter( | |||
There was a problem hiding this comment.
📝 notes: Note for myself. Time boxed the review to 1h and paused here for Today
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.
What Does This Do
Finishes context API migration in regards to
beforeFinishusing context instead of spanRefactors everything to use context instead of span across requests
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any usefull labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]