-
Notifications
You must be signed in to change notification settings - Fork 312
Support adding W3C baggage as span tags #9169
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?
Conversation
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
BenchmarksStartupParameters
See matching parameters
SummaryFound 10 performance improvements and 3 performance regressions! Performance is the same for 27 metrics, 13 unstable metrics.
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.52.0-SNAPSHOT~eca080eb87, baseline=1.52.0-SNAPSHOT~887ea397cc
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.044 s) : 0, 1043603
Total [baseline] (10.653 s) : 0, 10652615
Agent [candidate] (999.266 ms) : 0, 999266
Total [candidate] (10.642 s) : 0, 10642243
section appsec
Agent [baseline] (1.217 s) : 0, 1216762
Total [baseline] (10.767 s) : 0, 10767021
Agent [candidate] (1.181 s) : 0, 1181058
Total [candidate] (10.841 s) : 0, 10840958
section iast
Agent [baseline] (1.171 s) : 0, 1170694
Total [baseline] (10.865 s) : 0, 10864855
Agent [candidate] (1.145 s) : 0, 1144641
Total [candidate] (10.814 s) : 0, 10813774
section profiling
Agent [baseline] (1.184 s) : 0, 1184292
Total [baseline] (10.87 s) : 0, 10869745
Agent [candidate] (1.256 s) : 0, 1256301
Total [candidate] (11.058 s) : 0, 11058470
gantt
title petclinic - break down per module: candidate=1.52.0-SNAPSHOT~eca080eb87, baseline=1.52.0-SNAPSHOT~887ea397cc
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (733.25 ms) : 0, 733250
BytebuddyAgent [candidate] (687.824 ms) : 0, 687824
GlobalTracer [baseline] (242.31 ms) : 0, 242310
GlobalTracer [candidate] (244.681 ms) : 0, 244681
AppSec [baseline] (30.565 ms) : 0, 30565
AppSec [candidate] (30.826 ms) : 0, 30826
Debugger [baseline] (6.071 ms) : 0, 6071
Debugger [candidate] (6.064 ms) : 0, 6064
Remote Config [baseline] (670.398 µs) : 0, 670
Remote Config [candidate] (694.349 µs) : 0, 694
Telemetry [baseline] (8.287 ms) : 0, 8287
Telemetry [candidate] (8.276 ms) : 0, 8276
crashtracking [baseline] (1.444 ms) : 0, 1444
section appsec
BytebuddyAgent [baseline] (751.229 ms) : 0, 751229
BytebuddyAgent [candidate] (710.636 ms) : 0, 710636
GlobalTracer [baseline] (233.94 ms) : 0, 233940
GlobalTracer [candidate] (238.734 ms) : 0, 238734
AppSec [baseline] (168.117 ms) : 0, 168117
AppSec [candidate] (172.326 ms) : 0, 172326
Debugger [baseline] (8.502 ms) : 0, 8502
Debugger [candidate] (5.79 ms) : 0, 5790
Remote Config [baseline] (602.942 µs) : 0, 603
Remote Config [candidate] (614.014 µs) : 0, 614
Telemetry [baseline] (8.04 ms) : 0, 8040
Telemetry [candidate] (8.133 ms) : 0, 8133
crashtracking [baseline] (1.437 ms) : 0, 1437
IAST [baseline] (23.728 ms) : 0, 23728
IAST [candidate] (23.836 ms) : 0, 23836
section iast
BytebuddyAgent [baseline] (845.139 ms) : 0, 845139
BytebuddyAgent [candidate] (814.025 ms) : 0, 814025
GlobalTracer [baseline] (231.079 ms) : 0, 231079
GlobalTracer [candidate] (236.695 ms) : 0, 236695
AppSec [baseline] (27.255 ms) : 0, 27255
AppSec [candidate] (30.938 ms) : 0, 30938
Debugger [baseline] (7.48 ms) : 0, 7480
Debugger [candidate] (5.824 ms) : 0, 5824
Remote Config [baseline] (578.754 µs) : 0, 579
Remote Config [candidate] (588.615 µs) : 0, 589
Telemetry [baseline] (7.959 ms) : 0, 7959
Telemetry [candidate] (8.031 ms) : 0, 8031
crashtracking [baseline] (1.436 ms) : 0, 1436
IAST [baseline] (28.916 ms) : 0, 28916
IAST [candidate] (27.522 ms) : 0, 27522
section profiling
BytebuddyAgent [baseline] (759.412 ms) : 0, 759412
BytebuddyAgent [candidate] (682.19 ms) : 0, 682190
GlobalTracer [baseline] (220.334 ms) : 0, 220334
GlobalTracer [candidate] (365.939 ms) : 0, 365939
AppSec [baseline] (30.287 ms) : 0, 30287
AppSec [candidate] (32.125 ms) : 0, 32125
Debugger [baseline] (6.282 ms) : 0, 6282
Debugger [candidate] (11.347 ms) : 0, 11347
Remote Config [baseline] (687.121 µs) : 0, 687
Remote Config [candidate] (675.798 µs) : 0, 676
Telemetry [baseline] (11.198 ms) : 0, 11198
Telemetry [candidate] (9.648 ms) : 0, 9648
crashtracking [baseline] (1.403 ms) : 0, 1403
ProfilingAgent [baseline] (106.205 ms) : 0, 106205
ProfilingAgent [candidate] (105.377 ms) : 0, 105377
Profiling [baseline] (106.228 ms) : 0, 106228
Profiling [candidate] (105.401 ms) : 0, 105401
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.52.0-SNAPSHOT~eca080eb87, baseline=1.52.0-SNAPSHOT~887ea397cc
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.043 s) : 0, 1042635
Total [baseline] (8.675 s) : 0, 8675136
Agent [candidate] (996.97 ms) : 0, 996970
Total [candidate] (8.569 s) : 0, 8568788
section iast
Agent [baseline] (1.182 s) : 0, 1182249
Total [baseline] (9.37 s) : 0, 9370037
Agent [candidate] (1.135 s) : 0, 1135186
Total [candidate] (9.304 s) : 0, 9304271
gantt
title insecure-bank - break down per module: candidate=1.52.0-SNAPSHOT~eca080eb87, baseline=1.52.0-SNAPSHOT~887ea397cc
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (732.347 ms) : 0, 732347
BytebuddyAgent [candidate] (686.942 ms) : 0, 686942
GlobalTracer [baseline] (241.768 ms) : 0, 241768
GlobalTracer [candidate] (243.596 ms) : 0, 243596
AppSec [baseline] (30.494 ms) : 0, 30494
AppSec [candidate] (30.686 ms) : 0, 30686
Debugger [baseline] (5.971 ms) : 0, 5971
Debugger [candidate] (6.023 ms) : 0, 6023
Remote Config [baseline] (649.459 µs) : 0, 649
Remote Config [candidate] (680.267 µs) : 0, 680
Telemetry [baseline] (8.976 ms) : 0, 8976
Telemetry [candidate] (8.189 ms) : 0, 8189
crashtracking [baseline] (1.443 ms) : 0, 1443
section iast
BytebuddyAgent [baseline] (854.011 ms) : 0, 854011
BytebuddyAgent [candidate] (806.824 ms) : 0, 806824
GlobalTracer [baseline] (233.614 ms) : 0, 233614
GlobalTracer [candidate] (234.669 ms) : 0, 234669
AppSec [baseline] (28.27 ms) : 0, 28270
AppSec [candidate] (31.116 ms) : 0, 31116
Debugger [baseline] (7.513 ms) : 0, 7513
Debugger [candidate] (5.726 ms) : 0, 5726
Remote Config [baseline] (615.292 µs) : 0, 615
Remote Config [candidate] (575.721 µs) : 0, 576
Telemetry [baseline] (8.026 ms) : 0, 8026
Telemetry [candidate] (7.937 ms) : 0, 7937
crashtracking [baseline] (1.438 ms) : 0, 1438
IAST [baseline] (27.675 ms) : 0, 27675
IAST [candidate] (27.424 ms) : 0, 27424
LoadParameters
See matching parameters
SummaryFound 2 performance improvements and 2 performance regressions! Performance is the same for 8 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.52.0-SNAPSHOT~eca080eb87, baseline=1.52.0-SNAPSHOT~887ea397cc
dateFormat X
axisFormat %s
section baseline
no_agent (36.518 ms) : 36231, 36806
. : milestone, 36518,
appsec (49.936 ms) : 49487, 50384
. : milestone, 49936,
code_origins (44.226 ms) : 43854, 44599
. : milestone, 44226,
iast (45.764 ms) : 45373, 46154
. : milestone, 45764,
profiling (49.76 ms) : 49252, 50268
. : milestone, 49760,
tracing (44.253 ms) : 43873, 44633
. : milestone, 44253,
section candidate
no_agent (36.242 ms) : 35953, 36531
. : milestone, 36242,
appsec (47.829 ms) : 47403, 48254
. : milestone, 47829,
code_origins (46.748 ms) : 46348, 47148
. : milestone, 46748,
iast (44.019 ms) : 43624, 44413
. : milestone, 44019,
profiling (48.696 ms) : 48229, 49162
. : milestone, 48696,
tracing (45.139 ms) : 44757, 45522
. : milestone, 45139,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.52.0-SNAPSHOT~eca080eb87, baseline=1.52.0-SNAPSHOT~887ea397cc
dateFormat X
axisFormat %s
section baseline
no_agent (4.296 ms) : 4248, 4344
. : milestone, 4296,
iast (9.208 ms) : 9058, 9357
. : milestone, 9208,
iast_FULL (13.671 ms) : 13398, 13943
. : milestone, 13671,
iast_GLOBAL (10.244 ms) : 10065, 10423
. : milestone, 10244,
profiling (9.243 ms) : 9100, 9385
. : milestone, 9243,
tracing (7.713 ms) : 7602, 7825
. : milestone, 7713,
section candidate
no_agent (4.443 ms) : 4392, 4493
. : milestone, 4443,
iast (9.358 ms) : 9205, 9511
. : milestone, 9358,
iast_FULL (13.691 ms) : 13418, 13964
. : milestone, 13691,
iast_GLOBAL (10.237 ms) : 10054, 10419
. : milestone, 10237,
profiling (9.029 ms) : 8881, 9178
. : milestone, 9029,
tracing (7.784 ms) : 7665, 7904
. : milestone, 7784,
DacapoParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 0 unstable metrics.
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.52.0-SNAPSHOT~eca080eb87, baseline=1.52.0-SNAPSHOT~887ea397cc
dateFormat X
axisFormat %s
section baseline
no_agent (14.817 s) : 14817000, 14817000
. : milestone, 14817000,
appsec (14.603 s) : 14603000, 14603000
. : milestone, 14603000,
iast (18.439 s) : 18439000, 18439000
. : milestone, 18439000,
iast_GLOBAL (17.942 s) : 17942000, 17942000
. : milestone, 17942000,
profiling (16.067 s) : 16067000, 16067000
. : milestone, 16067000,
tracing (14.765 s) : 14765000, 14765000
. : milestone, 14765000,
section candidate
no_agent (14.832 s) : 14832000, 14832000
. : milestone, 14832000,
appsec (14.839 s) : 14839000, 14839000
. : milestone, 14839000,
iast (18.809 s) : 18809000, 18809000
. : milestone, 18809000,
iast_GLOBAL (18.112 s) : 18112000, 18112000
. : milestone, 18112000,
profiling (15.293 s) : 15293000, 15293000
. : milestone, 15293000,
tracing (14.661 s) : 14661000, 14661000
. : milestone, 14661000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.52.0-SNAPSHOT~eca080eb87, baseline=1.52.0-SNAPSHOT~887ea397cc
dateFormat X
axisFormat %s
section baseline
no_agent (1.475 ms) : 1464, 1487
. : milestone, 1475,
appsec (3.634 ms) : 3417, 3851
. : milestone, 3634,
iast (2.202 ms) : 2139, 2264
. : milestone, 2202,
iast_GLOBAL (2.248 ms) : 2185, 2311
. : milestone, 2248,
profiling (2.043 ms) : 1993, 2094
. : milestone, 2043,
tracing (2.01 ms) : 1961, 2059
. : milestone, 2010,
section candidate
no_agent (1.476 ms) : 1465, 1488
. : milestone, 1476,
appsec (2.414 ms) : 2363, 2464
. : milestone, 2414,
iast (2.201 ms) : 2138, 2263
. : milestone, 2201,
iast_GLOBAL (2.245 ms) : 2182, 2308
. : milestone, 2245,
profiling (2.04 ms) : 1990, 2090
. : milestone, 2040,
tracing (2.018 ms) : 1970, 2067
. : milestone, 2018,
|
dd-java-agent/instrumentation/okhttp-3/src/test/groovy/OkHttp3Test.groovy
Outdated
Show resolved
Hide resolved
…ext and update it to check the W3C baggage held in the new Context API (the baggage map in CoreTracer is OpenTracing only, and won't contain W3C baggage)
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 don't think it's the right place for baggage as tag injection.
I left some comments why and it might be the time to do a proper refactoring of this part.
cc @mhlidd about W3C Baggage vs OT Baggage lifecycle
if (this.tags == null) { | ||
this.tags = TagMap.create(4); | ||
} | ||
if (!this.tags.containsKey(key)) { | ||
this.tags.set(key, 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.
I would keep the tags
inflation in only place by reusing the existing putTag()
method:
if (this.tags == null) { | |
this.tags = TagMap.create(4); | |
} | |
if (!this.tags.containsKey(key)) { | |
this.tags.set(key, value); | |
} | |
} | |
if (this.tags == null || !this.tags.containsKey(key)) { | |
putTag(key, value); | |
} | |
} |
AgentSpanContext.Extracted extractedContext = null; | ||
if (extractedSpan != null) { | ||
extractedContext = (AgentSpanContext.Extracted) extractedSpan.context(); | ||
if (extractedContext instanceof TagContext) { | ||
Baggage baggage = Baggage.fromContext(context); | ||
if (baggage != null) { | ||
setBaggageTags((TagContext) extractedContext, baggage.asMap()); | ||
} | ||
} | ||
} | ||
return extractedContext; |
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 the right place for setting baggage?
I can think of the following issues:
- It's only for HttpServerDecorator while span and baggage will happen for every kind of instrumentation
getExtractedSpanContext
was a helper to extract span data from context, you don't expect it to do side effectgetExtractedSpanContext
can be called multiple time, calling the baggage injection as tag multiple time.
There are already parts of the code that does the "baggage as tag" thing, especially with OT baggage. It should be at the span serialization time. Can't we keep everything in this place?
I also added some related comment to @ygree PR about OT baggage injection PR. It might be the time to think more widely how OT and W3C must be applied as span tags. WDYT?
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.
There are already parts of the code that does the "baggage as tag" thing, especially with OT baggage. It should be at the span serialization time. Can't we keep everything in this place?
So the main issue is that we cannot extract W3C baggage into the existing OT baggage
map field in TagContext
because then the tracer would treat it as OT baggage to be propagated using OT headers later on, which would confuse users and waste bandwidth. (The logic of which baggage to add as tags and the key mapping is also completely different for OT vs W3C, which is another reason we'd can't use the existing field.)
We could attach the W3CBaggage
context to TagContext
under a new field, but the component creating the TagContext
is ContextInterpreter
which is used by HttpCodec.Extractor
and is disconnected from the new BaggageExtractor
. With the propagation code as it is today we'd still need to find a place where we could merge the TagContext
with the optional Baggage
context, which at the moment would be HttpServerDecorator
for the set of instrumentations covered by the RFC, since that's where the span context is extracted from Context
.
Otherwise we would have to wait for a refactoring which instead passes Context
into span creation, rather than AgentSpanContext.Extracted
- because the latter type only contains span-specific data.
If that refactoring is happening soon then it might be better to wait... /cc @mhlidd
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.
we cannot extract W3C baggage into the existing OT baggage map field in TagContext because then the tracer would treat it as OT baggage to be propagated using OT headers later on, which would confuse users and waste bandwidth.
Definitely agree that OT baggage is a separate concept and should not be mixed with W3C baggage. Hoping that it will be removed when we eventually end support for OT completely.
Otherwise we would have to wait for a refactoring which instead passes Context into span creation, rather than AgentSpanContext.Extracted - because the latter type only contains span-specific data.
FWIW the refactoring is expected to happen by EoQ. (I think it's assigned to @PerfectSlayer as a P1). I do think it would be much cleaner if we abstract all instrumentation-level span starting and baggage tag setting (among other instrumentation specific operations) into a single call that then calls the separate startSpan/set baggage tag operations.
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.
It's only for HttpServerDecorator while span and baggage will happen for every kind of instrumentation
As of now, Baggage is only supported for Http instrumentations. Since we (eventually) expect baggage to also be implemented in non-Http integrations, I wonder if the baggage tag helper should belong in ServerDecorator
instead? I wonder if there are downfalls to that approach.
What Does This Do
This PR introduces automatic adding of W3C baggage key-value pairs as span tags, prefixed with
baggage.
.The new configuration option
DD_TRACE_BAGGAGE_TAG_KEYS
allows customization of this feature. By default the following baggage entries are automatically added as span tags:user.id, session.id, account.id
See the related RFC for more details.
Motivation
Additional Notes
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: [PROJ-IDENT]