-
Notifications
You must be signed in to change notification settings - Fork 311
Improve Instrumenter API to use Context instead of Span #9211
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
Conversation
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 47 metrics, 12 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.53.0-SNAPSHOT~f7b9cb0fb1, baseline=1.53.0-SNAPSHOT~ef2e9f03e6
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.042 s) : 0, 1041519
Total [baseline] (8.56 s) : 0, 8560476
Agent [candidate] (1.051 s) : 0, 1050754
Total [candidate] (8.589 s) : 0, 8588917
section iast
Agent [baseline] (1.177 s) : 0, 1176841
Total [baseline] (9.299 s) : 0, 9298659
Agent [candidate] (1.174 s) : 0, 1173505
Total [candidate] (9.295 s) : 0, 9294868
gantt
title insecure-bank - break down per module: candidate=1.53.0-SNAPSHOT~f7b9cb0fb1, baseline=1.53.0-SNAPSHOT~ef2e9f03e6
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.414 ms) : 0, 1414
crashtracking [candidate] (1.449 ms) : 0, 1449
BytebuddyAgent [baseline] (729.993 ms) : 0, 729993
BytebuddyAgent [candidate] (736.095 ms) : 0, 736095
GlobalTracer [baseline] (241.299 ms) : 0, 241299
GlobalTracer [candidate] (243.237 ms) : 0, 243237
AppSec [baseline] (29.915 ms) : 0, 29915
AppSec [candidate] (30.275 ms) : 0, 30275
Debugger [baseline] (5.97 ms) : 0, 5970
Debugger [candidate] (6.081 ms) : 0, 6081
Remote Config [baseline] (652.953 µs) : 0, 653
Remote Config [candidate] (653.125 µs) : 0, 653
Telemetry [baseline] (11.4 ms) : 0, 11400
Telemetry [candidate] (12.041 ms) : 0, 12041
section iast
crashtracking [baseline] (1.422 ms) : 0, 1422
crashtracking [candidate] (1.421 ms) : 0, 1421
BytebuddyAgent [baseline] (849.401 ms) : 0, 849401
BytebuddyAgent [candidate] (847.045 ms) : 0, 847045
GlobalTracer [baseline] (232.543 ms) : 0, 232543
GlobalTracer [candidate] (231.847 ms) : 0, 231847
IAST [baseline] (26.992 ms) : 0, 26992
IAST [candidate] (29.172 ms) : 0, 29172
AppSec [baseline] (29.115 ms) : 0, 29115
AppSec [candidate] (27.657 ms) : 0, 27657
Debugger [baseline] (7.434 ms) : 0, 7434
Debugger [candidate] (6.591 ms) : 0, 6591
Remote Config [baseline] (582.4 µs) : 0, 582
Remote Config [candidate] (582.679 µs) : 0, 583
Telemetry [baseline] (8.333 ms) : 0, 8333
Telemetry [candidate] (8.28 ms) : 0, 8280
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.53.0-SNAPSHOT~f7b9cb0fb1, baseline=1.53.0-SNAPSHOT~ef2e9f03e6
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.045 s) : 0, 1044590
Total [baseline] (10.639 s) : 0, 10638581
Agent [candidate] (1.051 s) : 0, 1051285
Total [candidate] (10.701 s) : 0, 10700600
section appsec
Agent [baseline] (1.221 s) : 0, 1221168
Total [baseline] (10.821 s) : 0, 10820670
Agent [candidate] (1.219 s) : 0, 1218811
Total [candidate] (10.75 s) : 0, 10749961
section iast
Agent [baseline] (1.181 s) : 0, 1180524
Total [baseline] (10.908 s) : 0, 10907553
Agent [candidate] (1.18 s) : 0, 1179649
Total [candidate] (10.876 s) : 0, 10876013
section profiling
Agent [baseline] (1.208 s) : 0, 1207555
Total [baseline] (10.91 s) : 0, 10909854
Agent [candidate] (1.192 s) : 0, 1192431
Total [candidate] (10.844 s) : 0, 10844315
gantt
title petclinic - break down per module: candidate=1.53.0-SNAPSHOT~f7b9cb0fb1, baseline=1.53.0-SNAPSHOT~ef2e9f03e6
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.431 ms) : 0, 1431
crashtracking [candidate] (1.445 ms) : 0, 1445
BytebuddyAgent [baseline] (731.608 ms) : 0, 731608
BytebuddyAgent [candidate] (735.871 ms) : 0, 735871
GlobalTracer [baseline] (241.722 ms) : 0, 241722
GlobalTracer [candidate] (243.161 ms) : 0, 243161
AppSec [baseline] (30.085 ms) : 0, 30085
AppSec [candidate] (30.278 ms) : 0, 30278
Debugger [baseline] (6.005 ms) : 0, 6005
Debugger [candidate] (6.032 ms) : 0, 6032
Remote Config [baseline] (646.688 µs) : 0, 647
Remote Config [candidate] (660.571 µs) : 0, 661
Telemetry [baseline] (12.14 ms) : 0, 12140
Telemetry [candidate] (12.843 ms) : 0, 12843
section appsec
crashtracking [baseline] (1.434 ms) : 0, 1434
crashtracking [candidate] (1.432 ms) : 0, 1432
BytebuddyAgent [baseline] (753.841 ms) : 0, 753841
BytebuddyAgent [candidate] (753.032 ms) : 0, 753032
GlobalTracer [baseline] (235.489 ms) : 0, 235489
GlobalTracer [candidate] (234.324 ms) : 0, 234324
IAST [baseline] (23.744 ms) : 0, 23744
IAST [candidate] (23.39 ms) : 0, 23390
AppSec [baseline] (169.423 ms) : 0, 169423
AppSec [candidate] (168.572 ms) : 0, 168572
Debugger [baseline] (5.73 ms) : 0, 5730
Debugger [candidate] (7.995 ms) : 0, 7995
Remote Config [baseline] (613.468 µs) : 0, 613
Remote Config [candidate] (603.044 µs) : 0, 603
Telemetry [baseline] (9.881 ms) : 0, 9881
Telemetry [candidate] (8.372 ms) : 0, 8372
section iast
crashtracking [baseline] (1.429 ms) : 0, 1429
crashtracking [candidate] (1.434 ms) : 0, 1434
BytebuddyAgent [baseline] (852.202 ms) : 0, 852202
BytebuddyAgent [candidate] (851.188 ms) : 0, 851188
GlobalTracer [baseline] (233.571 ms) : 0, 233571
GlobalTracer [candidate] (233.592 ms) : 0, 233592
IAST [baseline] (30.035 ms) : 0, 30035
IAST [candidate] (26.853 ms) : 0, 26853
AppSec [baseline] (25.26 ms) : 0, 25260
AppSec [candidate] (29.986 ms) : 0, 29986
Debugger [baseline] (8.233 ms) : 0, 8233
Debugger [candidate] (6.584 ms) : 0, 6584
Remote Config [baseline] (588.853 µs) : 0, 589
Remote Config [candidate] (577.839 µs) : 0, 578
Telemetry [baseline] (8.238 ms) : 0, 8238
Telemetry [candidate] (8.291 ms) : 0, 8291
section profiling
crashtracking [baseline] (1.412 ms) : 0, 1412
crashtracking [candidate] (1.399 ms) : 0, 1399
BytebuddyAgent [baseline] (769.221 ms) : 0, 769221
BytebuddyAgent [candidate] (760.526 ms) : 0, 760526
GlobalTracer [baseline] (224.975 ms) : 0, 224975
GlobalTracer [candidate] (221.023 ms) : 0, 221023
AppSec [baseline] (30.611 ms) : 0, 30611
AppSec [candidate] (29.898 ms) : 0, 29898
Debugger [baseline] (6.399 ms) : 0, 6399
Debugger [candidate] (6.3 ms) : 0, 6300
Remote Config [baseline] (683.466 µs) : 0, 683
Remote Config [candidate] (698.006 µs) : 0, 698
Telemetry [baseline] (16.222 ms) : 0, 16222
Telemetry [candidate] (15.068 ms) : 0, 15068
ProfilingAgent [baseline] (108.117 ms) : 0, 108117
ProfilingAgent [candidate] (108.051 ms) : 0, 108051
Profiling [baseline] (108.771 ms) : 0, 108771
Profiling [candidate] (108.702 ms) : 0, 108702
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 1 performance regressions! Performance is the same for 10 metrics, 12 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.53.0-SNAPSHOT~f7b9cb0fb1, baseline=1.53.0-SNAPSHOT~ef2e9f03e6
dateFormat X
axisFormat %s
section baseline
no_agent (4.307 ms) : 4256, 4358
. : milestone, 4307,
iast (9.267 ms) : 9117, 9416
. : milestone, 9267,
iast_FULL (14.082 ms) : 13798, 14365
. : milestone, 14082,
iast_GLOBAL (10.455 ms) : 10269, 10641
. : milestone, 10455,
profiling (8.472 ms) : 8336, 8608
. : milestone, 8472,
tracing (7.607 ms) : 7500, 7715
. : milestone, 7607,
section candidate
no_agent (4.365 ms) : 4315, 4414
. : milestone, 4365,
iast (9.345 ms) : 9187, 9503
. : milestone, 9345,
iast_FULL (14.452 ms) : 14163, 14742
. : milestone, 14452,
iast_GLOBAL (10.449 ms) : 10266, 10633
. : milestone, 10449,
profiling (8.617 ms) : 8482, 8751
. : milestone, 8617,
tracing (7.838 ms) : 7723, 7953
. : milestone, 7838,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.53.0-SNAPSHOT~f7b9cb0fb1, baseline=1.53.0-SNAPSHOT~ef2e9f03e6
dateFormat X
axisFormat %s
section baseline
no_agent (36.993 ms) : 36689, 37297
. : milestone, 36993,
appsec (48.356 ms) : 47924, 48788
. : milestone, 48356,
code_origins (43.149 ms) : 42776, 43521
. : milestone, 43149,
iast (46.016 ms) : 45608, 46425
. : milestone, 46016,
profiling (52.041 ms) : 51565, 52516
. : milestone, 52041,
tracing (44.41 ms) : 44025, 44795
. : milestone, 44410,
section candidate
no_agent (37.48 ms) : 37183, 37778
. : milestone, 37480,
appsec (48.601 ms) : 48170, 49033
. : milestone, 48601,
code_origins (44.561 ms) : 44169, 44953
. : milestone, 44561,
iast (46.133 ms) : 45730, 46536
. : milestone, 46133,
profiling (48.765 ms) : 48311, 49218
. : milestone, 48765,
tracing (43.851 ms) : 43494, 44209
. : milestone, 43851,
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 tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.53.0-SNAPSHOT~f7b9cb0fb1, baseline=1.53.0-SNAPSHOT~ef2e9f03e6
dateFormat X
axisFormat %s
section baseline
no_agent (1.477 ms) : 1465, 1488
. : milestone, 1477,
appsec (3.69 ms) : 3471, 3909
. : milestone, 3690,
iast (2.2 ms) : 2137, 2262
. : milestone, 2200,
iast_GLOBAL (2.248 ms) : 2185, 2311
. : milestone, 2248,
profiling (2.07 ms) : 2017, 2122
. : milestone, 2070,
tracing (2.027 ms) : 1978, 2077
. : milestone, 2027,
section candidate
no_agent (1.476 ms) : 1464, 1487
. : milestone, 1476,
appsec (3.615 ms) : 3401, 3829
. : milestone, 3615,
iast (2.213 ms) : 2150, 2276
. : milestone, 2213,
iast_GLOBAL (2.257 ms) : 2193, 2320
. : milestone, 2257,
profiling (2.045 ms) : 1995, 2096
. : milestone, 2045,
tracing (2.029 ms) : 1980, 2078
. : milestone, 2029,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.53.0-SNAPSHOT~f7b9cb0fb1, baseline=1.53.0-SNAPSHOT~ef2e9f03e6
dateFormat X
axisFormat %s
section baseline
no_agent (14.96 s) : 14960000, 14960000
. : milestone, 14960000,
appsec (14.75 s) : 14750000, 14750000
. : milestone, 14750000,
iast (18.402 s) : 18402000, 18402000
. : milestone, 18402000,
iast_GLOBAL (17.985 s) : 17985000, 17985000
. : milestone, 17985000,
profiling (15.449 s) : 15449000, 15449000
. : milestone, 15449000,
tracing (14.846 s) : 14846000, 14846000
. : milestone, 14846000,
section candidate
no_agent (15.262 s) : 15262000, 15262000
. : milestone, 15262000,
appsec (14.917 s) : 14917000, 14917000
. : milestone, 14917000,
iast (18.361 s) : 18361000, 18361000
. : milestone, 18361000,
iast_GLOBAL (18.192 s) : 18192000, 18192000
. : milestone, 18192000,
profiling (15.333 s) : 15333000, 15333000
. : milestone, 15333000,
tracing (15.082 s) : 15082000, 15082000
. : milestone, 15082000,
|
b12a14b
to
8d4a9ea
Compare
Code coverage: total 57.31%, patch 76.92% (view details) This comment will be updated automatically if new data arrives.🔗 Commit SHA: f7b9cb0 | Docs | Was this helpful? Give us feedback! |
63e0f86
to
050278f
Compare
public static void blockAndThrowOnFailure( | ||
Request request, Response response, Flow.Action.RequestBlockingAction rba, AgentSpan span) { | ||
if (!block(request, response, rba, span)) { | ||
public static boolean hasRequestBlockingAction(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.
context
parameter could be removed here as always current but I find it more explicit to have it given as parameter as the context retrieval will be located near the context creation / attachement.
Additionally, all other helper methods have a Context
instance as parameter so it stays consistent.
@@ -216,4 +216,12 @@ default <T> T get(@Nonnull ContextKey<T> key) { | |||
default <T> Context with(@Nonnull ContextKey<T> key, @Nullable T value) { | |||
return SPAN_KEY == key ? (Context) value : Context.root().with(SPAN_KEY, this, key, value); | |||
} | |||
|
|||
@Override | |||
default Context with(@Nullable ImplicitContextKeyed value) { |
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 optimization usually happens when creating local root span from remote extracted context.
@mcculls Do you think it worth it as we will end up allocating an array as soon as there is a second context element?
… of span on startSpan
… of span on startSpan
Clean up local var hook and use Context.current() instead Refactor helper to use Context instead of AgentSpan and RequestBlockingAction
Clean up local var hook and use Context.current() instead Refactor helper to use Context instead of AgentSpan and RequestBlockingAction
Clean up local var hook and use Context.current() instead Refactor helper to use Context instead of AgentSpan and RequestBlockingAction
41ab563
to
57e8038
Compare
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.
Overall looks good! Left a few notes
@@ -160,6 +164,11 @@ public AgentSpan onRequest( | |||
return onRequest(span, connection, request, getExtractedSpanContext(context)); | |||
} | |||
|
|||
public AgentSpanContext.Extracted getExtractedSpanContext(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.
Is this something that we can set as a private method now? It seems like the only two instrumentations that use this still are SprayHttpServerRunSealedRouteAdvice
and TomcatServerInstrumentation
. (Albeit modifications would have to be made to those functionality)
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.
About TomcatServerInstrumentation
, it feels I would be picking your leftover (git blame this line 😬 ):
Line 127 in 1416a77
// TODO: Migrate setting DD_EXTRACTED_CONTEXT_ATTRIBUTE from AgentSpanContext.Extracted to |
About Spray, I could try. I'm not that familiar with Scala, but that sounds doable.
What about doing it in a follow up PR? I expect to have even more refactoring coming to make gateway inferred span happen.
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.
Follow up on #9358
} else { | ||
span = startSpan(DECORATE.spanName()); | ||
span.setMeasured(true); | ||
context = 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.
Does it make sense to do a check for context == Context.root()
and if so then call startSpan(DECORATE.spanName());
?
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.
Not really. spanSpan
can be called with a context without tracing span.
Even the IG callback support the fact there is no Extracted
context.
The goal of having non-null, immutable, always-valid context instances is to avoid this kind of branching and make the code simpler.
.../restlet-2.2/src/main/java/datadog/trace/instrumentation/restlet/RestletInstrumentation.java
Show resolved
Hide resolved
.../instrumentation/play-2.6/src/main/java/datadog/trace/instrumentation/play26/PlayAdvice.java
Show resolved
Hide resolved
.../src/test/groovy/datadog/trace/instrumentation/jetty/JettyBlockingHelperSpecification.groovy
Show resolved
Hide resolved
...tion/jetty-common/src/main/java/datadog/trace/instrumentation/jetty/JettyBlockingHelper.java
Outdated
Show resolved
Hide resolved
@@ -1,4 +1,4 @@ | |||
package datadog.trace.instrumentation.azurefunctions; | |||
package datadog.trace.instrumentation.azure.functions; |
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.
Wow good catch 😮
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.
Yeah, I need to review the spec to find out how this even work 🤷
...rap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/HttpServerDecorator.java
Outdated
Show resolved
Hide resolved
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.
Looks ok. Thanks for the detailed description
5412294
to
32b8a2a
Compare
Unclear of the benefits compared to the existing optimization.
What Does This Do
Improve the instrumenter API to be context-based rather than span-based.
This will allow products other than tracing to create and aggregate information that does no more have to be stored in span. The main benefit is tracing instrumentations won't always have to be enabled so products do no more need pay the tracing overhead when trying to keep track of their data.
startSpan
API fromHttpServerDecorator
to be based onContext
API. It comes with the following names for clarity:parentContext
as the upstream service context, usually contains anExtracted
context as tracing contextcontext
as the new child context containing tracing, dsm, asm information.startSpan
uses the instrumentation name declared from theHttpServerDecorator
implementationContext
rather thanAgentSpan
andRequestBlockingAction
CorrelationIdentifier
API to get key and values to store for correlationdatadog.trace.instrumentation.azurefunctions
package nameMotivation
AppSec changes was required as it was instrumenting the
HttpServerDecorator
API and it would help to move toward the Context API.It simplifies both the call (less stack changes) and the null checks (less instructions to inject).
It also moves from local variable index lookup to idempotent access to current context (attach few instructions below).
It simplifies the
DelayCertainInsMethodVisitor
to useRunnable
instead ofFunction
as reapplying visitations do not requires parameters nor return values.Additional Notes
Any minor change to the
HttpServerDecorator
needs a lot of fixes in multiple files.I would have a hard time trying to split the PR into smaller working chunks.
Contributor Checklist
type:
and (comp:
orinst:
) labels in addition to any usefull labelsclose
,fix
or any linking keywords when referencing an issue.Use
solves
instead, and assign the PR milestone to the issueJira ticket: LANGPLAT-680