-
Notifications
You must be signed in to change notification settings - Fork 292
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
Updating Span Link creation due to header tag propagations for invalid spans #7799
Conversation
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 53 metrics, 10 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.43.0-SNAPSHOT~e3eb2e0459, baseline=1.42.0-SNAPSHOT~6eac0b8465
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.087 s) : 0, 1086775
Total [baseline] (8.594 s) : 0, 8594493
Agent [candidate] (1.085 s) : 0, 1084973
Total [candidate] (8.596 s) : 0, 8596058
section iast
Agent [baseline] (1.217 s) : 0, 1217320
Total [baseline] (9.206 s) : 0, 9206117
Agent [candidate] (1.21 s) : 0, 1210137
Total [candidate] (9.165 s) : 0, 9164501
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.211 s) : 0, 1210684
Total [baseline] (9.191 s) : 0, 9190637
Agent [candidate] (1.209 s) : 0, 1209082
Total [candidate] (9.127 s) : 0, 9127427
section iast_TELEMETRY_OFF
Agent [baseline] (1.207 s) : 0, 1206696
Total [baseline] (9.152 s) : 0, 9151996
Agent [candidate] (1.223 s) : 0, 1223084
Total [candidate] (9.202 s) : 0, 9202218
gantt
title insecure-bank - break down per module: candidate=1.43.0-SNAPSHOT~e3eb2e0459, baseline=1.42.0-SNAPSHOT~6eac0b8465
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (689.671 ms) : 0, 689671
BytebuddyAgent [candidate] (689.068 ms) : 0, 689068
GlobalTracer [baseline] (317.73 ms) : 0, 317730
GlobalTracer [candidate] (316.865 ms) : 0, 316865
AppSec [baseline] (54.333 ms) : 0, 54333
AppSec [candidate] (54.619 ms) : 0, 54619
Remote Config [baseline] (666.303 µs) : 0, 666
Remote Config [candidate] (677.146 µs) : 0, 677
Telemetry [baseline] (10.588 ms) : 0, 10588
Telemetry [candidate] (9.95 ms) : 0, 9950
section iast
BytebuddyAgent [baseline] (809.402 ms) : 0, 809402
BytebuddyAgent [candidate] (804.158 ms) : 0, 804158
GlobalTracer [baseline] (307.107 ms) : 0, 307107
GlobalTracer [candidate] (305.554 ms) : 0, 305554
AppSec [baseline] (56.407 ms) : 0, 56407
AppSec [candidate] (56.86 ms) : 0, 56860
Remote Config [baseline] (624.581 µs) : 0, 625
Remote Config [candidate] (619.955 µs) : 0, 620
Telemetry [baseline] (7.464 ms) : 0, 7464
Telemetry [candidate] (7.479 ms) : 0, 7479
IAST [baseline] (22.461 ms) : 0, 22461
IAST [candidate] (21.637 ms) : 0, 21637
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (803.622 ms) : 0, 803622
BytebuddyAgent [candidate] (803.543 ms) : 0, 803543
GlobalTracer [baseline] (305.652 ms) : 0, 305652
GlobalTracer [candidate] (305.685 ms) : 0, 305685
AppSec [baseline] (57.455 ms) : 0, 57455
AppSec [candidate] (57.153 ms) : 0, 57153
Remote Config [baseline] (642.095 µs) : 0, 642
Remote Config [candidate] (629.445 µs) : 0, 629
Telemetry [baseline] (7.681 ms) : 0, 7681
Telemetry [candidate] (7.54 ms) : 0, 7540
IAST [baseline] (21.855 ms) : 0, 21855
IAST [candidate] (20.719 ms) : 0, 20719
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (801.452 ms) : 0, 801452
BytebuddyAgent [candidate] (812.962 ms) : 0, 812962
GlobalTracer [baseline] (305.221 ms) : 0, 305221
GlobalTracer [candidate] (309.559 ms) : 0, 309559
AppSec [baseline] (58.053 ms) : 0, 58053
AppSec [candidate] (57.155 ms) : 0, 57155
Remote Config [baseline] (606.723 µs) : 0, 607
Remote Config [candidate] (618.192 µs) : 0, 618
Telemetry [baseline] (7.432 ms) : 0, 7432
Telemetry [candidate] (7.443 ms) : 0, 7443
IAST [baseline] (20.161 ms) : 0, 20161
IAST [candidate] (21.332 ms) : 0, 21332
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.43.0-SNAPSHOT~e3eb2e0459, baseline=1.42.0-SNAPSHOT~6eac0b8465
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.086 s) : 0, 1086180
Total [baseline] (10.493 s) : 0, 10492980
Agent [candidate] (1.099 s) : 0, 1098927
Total [candidate] (10.461 s) : 0, 10460618
section appsec
Agent [baseline] (1.221 s) : 0, 1221105
Total [baseline] (10.705 s) : 0, 10704883
Agent [candidate] (1.221 s) : 0, 1220513
Total [candidate] (10.67 s) : 0, 10670079
section iast
Agent [baseline] (1.208 s) : 0, 1208144
Total [baseline] (10.885 s) : 0, 10885186
Agent [candidate] (1.217 s) : 0, 1217290
Total [candidate] (11.011 s) : 0, 11011210
section profiling
Agent [baseline] (1.282 s) : 0, 1281962
Total [baseline] (10.784 s) : 0, 10783771
Agent [candidate] (1.28 s) : 0, 1280300
Total [candidate] (10.813 s) : 0, 10812711
gantt
title petclinic - break down per module: candidate=1.43.0-SNAPSHOT~e3eb2e0459, baseline=1.42.0-SNAPSHOT~6eac0b8465
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (689.072 ms) : 0, 689072
BytebuddyAgent [candidate] (697.741 ms) : 0, 697741
GlobalTracer [baseline] (317.284 ms) : 0, 317284
GlobalTracer [candidate] (320.947 ms) : 0, 320947
AppSec [baseline] (54.71 ms) : 0, 54710
AppSec [candidate] (54.876 ms) : 0, 54876
Remote Config [baseline] (665.493 µs) : 0, 665
Remote Config [candidate] (674.517 µs) : 0, 675
Telemetry [baseline] (10.717 ms) : 0, 10717
Telemetry [candidate] (10.755 ms) : 0, 10755
section appsec
BytebuddyAgent [baseline] (707.641 ms) : 0, 707641
BytebuddyAgent [candidate] (706.967 ms) : 0, 706967
GlobalTracer [baseline] (314.859 ms) : 0, 314859
GlobalTracer [candidate] (314.953 ms) : 0, 314953
AppSec [baseline] (165.316 ms) : 0, 165316
AppSec [candidate] (166.991 ms) : 0, 166991
IAST [baseline] (21.441 ms) : 0, 21441
IAST [candidate] (19.474 ms) : 0, 19474
Remote Config [baseline] (640.115 µs) : 0, 640
Remote Config [candidate] (634.74 µs) : 0, 635
Telemetry [baseline] (8.155 ms) : 0, 8155
Telemetry [candidate] (7.449 ms) : 0, 7449
section iast
BytebuddyAgent [baseline] (803.247 ms) : 0, 803247
BytebuddyAgent [candidate] (809.727 ms) : 0, 809727
GlobalTracer [baseline] (304.707 ms) : 0, 304707
GlobalTracer [candidate] (307.233 ms) : 0, 307233
AppSec [baseline] (56.938 ms) : 0, 56938
AppSec [candidate] (57.514 ms) : 0, 57514
IAST [baseline] (21.411 ms) : 0, 21411
IAST [candidate] (20.778 ms) : 0, 20778
Remote Config [baseline] (617.45 µs) : 0, 617
Remote Config [candidate] (623.473 µs) : 0, 623
Telemetry [baseline] (7.488 ms) : 0, 7488
Telemetry [candidate] (7.555 ms) : 0, 7555
section profiling
BytebuddyAgent [baseline] (681.439 ms) : 0, 681439
BytebuddyAgent [candidate] (681.318 ms) : 0, 681318
GlobalTracer [baseline] (401.039 ms) : 0, 401039
GlobalTracer [candidate] (400.121 ms) : 0, 400121
AppSec [baseline] (54.558 ms) : 0, 54558
AppSec [candidate] (54.627 ms) : 0, 54627
Remote Config [baseline] (680.017 µs) : 0, 680
Remote Config [candidate] (673.006 µs) : 0, 673
Telemetry [baseline] (12.847 ms) : 0, 12847
Telemetry [candidate] (12.804 ms) : 0, 12804
ProfilingAgent [baseline] (92.249 ms) : 0, 92249
ProfilingAgent [candidate] (91.678 ms) : 0, 91678
Profiling [baseline] (92.272 ms) : 0, 92272
Profiling [candidate] (91.701 ms) : 0, 91701
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 7 metrics, 21 unstable metrics. Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.43.0-SNAPSHOT~e3eb2e0459, baseline=1.42.0-SNAPSHOT~6eac0b8465
dateFormat X
axisFormat %s
section baseline
no_agent (446.44 µs) : 417, 476
. : milestone, 446,
iast (584.282 µs) : 553, 616
. : milestone, 584,
iast_FULL (832.983 µs) : 800, 866
. : milestone, 833,
iast_GLOBAL (618.0 µs) : 586, 650
. : milestone, 618,
iast_HARDCODED_SECRET_DISABLED (585.337 µs) : 552, 619
. : milestone, 585,
iast_INACTIVE (540.769 µs) : 510, 571
. : milestone, 541,
iast_TELEMETRY_OFF (578.799 µs) : 546, 611
. : milestone, 579,
tracing (533.257 µs) : 503, 563
. : milestone, 533,
section candidate
no_agent (448.604 µs) : 420, 477
. : milestone, 449,
iast (584.267 µs) : 551, 617
. : milestone, 584,
iast_FULL (835.233 µs) : 802, 868
. : milestone, 835,
iast_GLOBAL (624.444 µs) : 593, 656
. : milestone, 624,
iast_HARDCODED_SECRET_DISABLED (588.28 µs) : 555, 621
. : milestone, 588,
iast_INACTIVE (539.49 µs) : 509, 570
. : milestone, 539,
iast_TELEMETRY_OFF (574.063 µs) : 542, 606
. : milestone, 574,
tracing (532.607 µs) : 503, 563
. : milestone, 533,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.43.0-SNAPSHOT~e3eb2e0459, baseline=1.42.0-SNAPSHOT~6eac0b8465
dateFormat X
axisFormat %s
section baseline
no_agent (1.686 ms) : 1661, 1712
. : milestone, 1686,
appsec (2.192 ms) : 2161, 2224
. : milestone, 2192,
appsec_no_iast (2.183 ms) : 2151, 2215
. : milestone, 2183,
iast (1.86 ms) : 1830, 1890
. : milestone, 1860,
profiling (1.955 ms) : 1921, 1988
. : milestone, 1955,
tracing (1.843 ms) : 1812, 1874
. : milestone, 1843,
section candidate
no_agent (1.697 ms) : 1672, 1721
. : milestone, 1697,
appsec (2.195 ms) : 2165, 2225
. : milestone, 2195,
appsec_no_iast (2.193 ms) : 2161, 2224
. : milestone, 2193,
iast (1.873 ms) : 1843, 1903
. : milestone, 1873,
profiling (1.968 ms) : 1935, 2002
. : milestone, 1968,
tracing (1.877 ms) : 1845, 1909
. : milestone, 1877,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics. Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.43.0-SNAPSHOT~e3eb2e0459, baseline=1.42.0-SNAPSHOT~6eac0b8465
dateFormat X
axisFormat %s
section baseline
no_agent (1.463 ms) : 1452, 1474
. : milestone, 1463,
appsec (2.341 ms) : 2299, 2382
. : milestone, 2341,
iast (2.085 ms) : 2032, 2138
. : milestone, 2085,
iast_GLOBAL (2.136 ms) : 2083, 2189
. : milestone, 2136,
profiling (1.951 ms) : 1909, 1993
. : milestone, 1951,
tracing (1.924 ms) : 1884, 1964
. : milestone, 1924,
section candidate
no_agent (1.467 ms) : 1456, 1478
. : milestone, 1467,
appsec (2.356 ms) : 2315, 2398
. : milestone, 2356,
iast (2.077 ms) : 2026, 2129
. : milestone, 2077,
iast_GLOBAL (2.139 ms) : 2086, 2191
. : milestone, 2139,
profiling (1.956 ms) : 1914, 1998
. : milestone, 1956,
tracing (1.925 ms) : 1885, 1965
. : milestone, 1925,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.43.0-SNAPSHOT~e3eb2e0459, baseline=1.42.0-SNAPSHOT~6eac0b8465
dateFormat X
axisFormat %s
section baseline
no_agent (15.076 s) : 15076000, 15076000
. : milestone, 15076000,
appsec (15.13 s) : 15130000, 15130000
. : milestone, 15130000,
iast (18.79 s) : 18790000, 18790000
. : milestone, 18790000,
iast_GLOBAL (18.139 s) : 18139000, 18139000
. : milestone, 18139000,
profiling (14.718 s) : 14718000, 14718000
. : milestone, 14718000,
tracing (15.057 s) : 15057000, 15057000
. : milestone, 15057000,
section candidate
no_agent (15.664 s) : 15664000, 15664000
. : milestone, 15664000,
appsec (14.959 s) : 14959000, 14959000
. : milestone, 14959000,
iast (18.621 s) : 18621000, 18621000
. : milestone, 18621000,
iast_GLOBAL (18.229 s) : 18229000, 18229000
. : milestone, 18229000,
profiling (15.775 s) : 15775000, 15775000
. : milestone, 15775000,
tracing (15.281 s) : 15281000, 15281000
. : milestone, 15281000,
|
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.
Nice additions. I left few questions during the review.
The way that the Java library is sending the traceFlags field is unclear, as there is no real way to differentiate whether a sampling flag is unset, or manually set to 0.
I’m here if you would like to talk about it. Basically, trace context flags are a set a flags, encoding using bit masks on 1 byte type. There is currently only one flag supported, called sampled.
So, there is no such things as set the sampling flag to 0, it’s either the sampled flag is set, or the sampled flag is not set.
Settings the sampled flag will toggle the first bit of the byte to 1. Not settings the sampled flag will leave the byte to 0.
dd-trace-core/src/main/java/datadog/trace/core/propagation/HttpCodec.java
Outdated
Show resolved
Hide resolved
dd-trace-core/src/main/java/datadog/trace/core/propagation/HttpCodec.java
Outdated
Show resolved
Hide resolved
dd-trace-core/src/test/groovy/datadog/trace/core/propagation/HttpExtractorTest.groovy
Outdated
Show resolved
Hide resolved
context.addTerminatedContextLink(DDSpanLink.from((ExtractedContext) extracted)); | ||
Map<String, String> attributes = new HashMap<>(); | ||
attributes.put("reason", "terminated_context"); | ||
attributes.put("context_headers", extracted.getPropagationStyle().toString()); |
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.
Dumb question: where is the spec for this part? Because at some point, context won’t always come from "headers" but message properties too, etc...
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 are only worried about extracting information from headers here and creating span links for those headers that have inconsistent trace ids. (RFC)
I think the important distinction to be made here is that there is a difference between not sampling and sampling unset, since the DD agent may update the sampling state after the span is created if the sampling is unset. Thus, it would be nice to be able to differentiate whether a span is not sampled vs sampling unset. If both are set to 0, there is no way to differentiate the two in the backend, which is also the reason that the bit-shifting was important for v04 encodings. This isn't a feature that is critical to this task, but is more of a nice to have. |
What you are describing is the
The v0.4 protocol workaround should not impact the JSON encoding. If they ever come up with a drop flag, we will implement it :) |
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.
LGTM 👍
Don't forget to squash on merge 😉
What Does This Do
Updates the keys in the Span Link creation to include the
attributes
field and to only includetracestate
when the propagation style istracecontext
. Additionally, wrote a unit test to verify the creation of Span Links when spans passed down through header tags are "invalid" due to differing trace_ids.Motivation
Currently the implementation of how Span Links are used when DD receives invalid traces are not consistent among libraries. This PR focuses on making the Java library's implementation of Span Links consistent with the following RFC.
Additional Notes
The way that the Java library is sending the
traceFlags
field is unclear, as there is no real way to differentiate whether a sampling flag is unset, or manually set to 0.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]