-
Notifications
You must be signed in to change notification settings - Fork 315
Normalizing Behavior of OTel Spans Created From Custom-Instrumentation and Trace Annotations #9759
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
|
🎯 Code Coverage 🔗 Commit SHA: 7943bc9 | Docs | Was this helpful? Give us feedback! |
| private static String convertToSpanType(SpanKind kind) { | ||
| if (kind == null) { | ||
| return null; | ||
| } | ||
| switch (kind) { | ||
| case SERVER: | ||
| return HTTP_SERVER; | ||
| case CLIENT: | ||
| return HTTP_CLIENT; | ||
| case PRODUCER: | ||
| return MESSAGE_PRODUCER; | ||
| case CONSUMER: | ||
| return MESSAGE_CONSUMER; | ||
| case INTERNAL: | ||
| // checking for SpanKind.INTERNAL, returning DDSpanTypes.INTERNAL | ||
| return INTERNAL; | ||
| default: | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| private static String convertToSpanKindTag(SpanKind kind) { | ||
| if (kind == null) { | ||
| return null; | ||
| } | ||
| switch (kind) { | ||
| case CLIENT: | ||
| return SPAN_KIND_CLIENT; | ||
| case SERVER: | ||
| return SPAN_KIND_SERVER; | ||
| case PRODUCER: | ||
| return SPAN_KIND_PRODUCER; | ||
| case CONSUMER: | ||
| return SPAN_KIND_CONSUMER; | ||
| case INTERNAL: | ||
| return SPAN_KIND_INTERNAL; | ||
| default: | ||
| return null; | ||
| } |
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 is currently duplicated code between the opentelemetry-shim and opentelemetry-annotation modules. @PerfectSlayer what do you think about introducing a new opentelemetry-utils module in utils for shared helpers? 🤔
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.
Note that the otel-shim classes are already duplicated at build-time and placed under different packages - i.e. one source, but made available in two places in the final jar. This is to support both OTel drop-in support (which cannot expose the OTel packages on the classpath, so must repackage classes both at build-time and run-time) but also instrument OTel client code in the application.
I would investigate whether the annotations instrumentation can also depend on
implementation project(':dd-java-agent:agent-otel:otel-shim')
like the main OTel instrumentation under opentelemetry-1.4
PerfectSlayer
left a comment
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: My main question for this change is should we change this behavior as it is not part of the spec, or should we evolve the spec and make sure the behavior is consistent across all languages? -- system tests could help here too.
Because I can think of case where customers set span type on purpose and it gets overridden when setting span kind.
| public static final String CACHE = "cache"; | ||
| public static final String SOAP = "soap"; | ||
|
|
||
| public static final String INTERNAL = "internal"; |
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: It looks like a duplicate of datadog.trace.bootstrap.instrumentation.api.Tags#SPAN_KIND_INTERNAL
| * @param kind The OpenTelemetry span kind to convert. | ||
| * @return The {@link DDSpanTypes} value. | ||
| */ | ||
| public static String convertToSpanType(SpanKind kind) { |
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: To keep method naming consistent:
| public static String convertToSpanType(SpanKind kind) { | |
| public static String toSpanType(SpanKind kind) { |
| if (kind == null) { | ||
| return null; | ||
| } |
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: null is not supposed to be given as parameter as all calls are guarded.
Similarly, I would not have a default case returning null but internal.
I think you are right that this change deviates from the spec. There are 2 concepts that are closely tied together, which makes this situation confusing. Below are the changes I think actually should be done:
I looked into the OTel implementation of using a What do you think @PerfectSlayer? I can see a world where we should keep the legacy behavior of |
No I think it make sens to change to support both span kind and span type on OTel Tracing API and annotation support. One thing that might be missing is the implementation of the |
aba17be to
b46811b
Compare
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 60 metrics, 5 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.55.0-SNAPSHOT~7943bc960c, baseline=1.55.0-SNAPSHOT~60c4f68423
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.02 s) : 0, 1020214
Total [baseline] (8.67 s) : 0, 8670283
Agent [candidate] (1.018 s) : 0, 1017925
Total [candidate] (8.686 s) : 0, 8685596
section iast
Agent [baseline] (1.152 s) : 0, 1152126
Total [baseline] (9.296 s) : 0, 9296100
Agent [candidate] (1.16 s) : 0, 1159544
Total [candidate] (9.318 s) : 0, 9318067
gantt
title insecure-bank - break down per module: candidate=1.55.0-SNAPSHOT~7943bc960c, baseline=1.55.0-SNAPSHOT~60c4f68423
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.464 ms) : 0, 1464
crashtracking [candidate] (1.457 ms) : 0, 1457
BytebuddyAgent [baseline] (695.565 ms) : 0, 695565
BytebuddyAgent [candidate] (692.641 ms) : 0, 692641
GlobalTracer [baseline] (242.564 ms) : 0, 242564
GlobalTracer [candidate] (243.566 ms) : 0, 243566
AppSec [baseline] (32.309 ms) : 0, 32309
AppSec [candidate] (32.524 ms) : 0, 32524
Debugger [baseline] (6.299 ms) : 0, 6299
Debugger [candidate] (6.334 ms) : 0, 6334
Remote Config [baseline] (689.822 µs) : 0, 690
Remote Config [candidate] (685.602 µs) : 0, 686
Telemetry [baseline] (9.271 ms) : 0, 9271
Telemetry [candidate] (9.347 ms) : 0, 9347
Flare Poller [baseline] (10.884 ms) : 0, 10884
Flare Poller [candidate] (10.26 ms) : 0, 10260
section iast
crashtracking [baseline] (1.473 ms) : 0, 1473
crashtracking [candidate] (1.487 ms) : 0, 1487
BytebuddyAgent [baseline] (815.956 ms) : 0, 815956
BytebuddyAgent [candidate] (820.773 ms) : 0, 820773
GlobalTracer [baseline] (231.644 ms) : 0, 231644
GlobalTracer [candidate] (233.01 ms) : 0, 233010
IAST [baseline] (26.668 ms) : 0, 26668
IAST [candidate] (27.151 ms) : 0, 27151
AppSec [baseline] (35.285 ms) : 0, 35285
AppSec [candidate] (35.444 ms) : 0, 35444
Debugger [baseline] (6.159 ms) : 0, 6159
Debugger [candidate] (6.291 ms) : 0, 6291
Remote Config [baseline] (610.494 µs) : 0, 610
Remote Config [candidate] (631.79 µs) : 0, 632
Telemetry [baseline] (8.755 ms) : 0, 8755
Telemetry [candidate] (8.859 ms) : 0, 8859
Flare Poller [baseline] (4.208 ms) : 0, 4208
Flare Poller [candidate] (4.272 ms) : 0, 4272
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.55.0-SNAPSHOT~7943bc960c, baseline=1.55.0-SNAPSHOT~60c4f68423
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.021 s) : 0, 1021274
Total [baseline] (10.656 s) : 0, 10655677
Agent [candidate] (1.02 s) : 0, 1020125
Total [candidate] (10.75 s) : 0, 10750317
section appsec
Agent [baseline] (1.197 s) : 0, 1196586
Total [baseline] (10.826 s) : 0, 10825846
Agent [candidate] (1.215 s) : 0, 1215015
Total [candidate] (11.013 s) : 0, 11013308
section iast
Agent [baseline] (1.152 s) : 0, 1151859
Total [baseline] (11.102 s) : 0, 11102393
Agent [candidate] (1.156 s) : 0, 1156347
Total [candidate] (11.068 s) : 0, 11067662
section profiling
Agent [baseline] (1.162 s) : 0, 1161932
Total [baseline] (10.848 s) : 0, 10847788
Agent [candidate] (1.163 s) : 0, 1163173
Total [candidate] (10.817 s) : 0, 10817332
gantt
title petclinic - break down per module: candidate=1.55.0-SNAPSHOT~7943bc960c, baseline=1.55.0-SNAPSHOT~60c4f68423
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.468 ms) : 0, 1468
crashtracking [candidate] (1.457 ms) : 0, 1457
BytebuddyAgent [baseline] (695.365 ms) : 0, 695365
BytebuddyAgent [candidate] (695.253 ms) : 0, 695253
GlobalTracer [baseline] (242.906 ms) : 0, 242906
GlobalTracer [candidate] (243.364 ms) : 0, 243364
AppSec [baseline] (32.209 ms) : 0, 32209
AppSec [candidate] (32.282 ms) : 0, 32282
Debugger [baseline] (6.295 ms) : 0, 6295
Debugger [candidate] (6.318 ms) : 0, 6318
Remote Config [baseline] (678.059 µs) : 0, 678
Remote Config [candidate] (679.62 µs) : 0, 680
Telemetry [baseline] (9.322 ms) : 0, 9322
Telemetry [candidate] (9.179 ms) : 0, 9179
Flare Poller [baseline] (11.809 ms) : 0, 11809
Flare Poller [candidate] (10.356 ms) : 0, 10356
section appsec
crashtracking [baseline] (1.461 ms) : 0, 1461
crashtracking [candidate] (1.467 ms) : 0, 1467
BytebuddyAgent [baseline] (719.417 ms) : 0, 719417
BytebuddyAgent [candidate] (730.555 ms) : 0, 730555
GlobalTracer [baseline] (235.057 ms) : 0, 235057
GlobalTracer [candidate] (239.161 ms) : 0, 239161
IAST [baseline] (24.95 ms) : 0, 24950
IAST [candidate] (25.524 ms) : 0, 25524
AppSec [baseline] (174.547 ms) : 0, 174547
AppSec [candidate] (177.319 ms) : 0, 177319
Debugger [baseline] (6.033 ms) : 0, 6033
Debugger [candidate] (6.233 ms) : 0, 6233
Remote Config [baseline] (633.229 µs) : 0, 633
Remote Config [candidate] (642.935 µs) : 0, 643
Telemetry [baseline] (8.547 ms) : 0, 8547
Telemetry [candidate] (8.691 ms) : 0, 8691
Flare Poller [baseline] (4.734 ms) : 0, 4734
Flare Poller [candidate] (3.999 ms) : 0, 3999
section iast
crashtracking [baseline] (1.46 ms) : 0, 1460
crashtracking [candidate] (1.455 ms) : 0, 1455
BytebuddyAgent [baseline] (815.586 ms) : 0, 815586
BytebuddyAgent [candidate] (818.269 ms) : 0, 818269
GlobalTracer [baseline] (231.498 ms) : 0, 231498
GlobalTracer [candidate] (232.867 ms) : 0, 232867
IAST [baseline] (26.813 ms) : 0, 26813
IAST [candidate] (26.922 ms) : 0, 26922
AppSec [baseline] (35.146 ms) : 0, 35146
AppSec [candidate] (35.378 ms) : 0, 35378
Debugger [baseline] (6.186 ms) : 0, 6186
Debugger [candidate] (6.212 ms) : 0, 6212
Remote Config [baseline] (618.555 µs) : 0, 619
Remote Config [candidate] (646.217 µs) : 0, 646
Telemetry [baseline] (8.821 ms) : 0, 8821
Telemetry [candidate] (8.793 ms) : 0, 8793
Flare Poller [baseline] (4.194 ms) : 0, 4194
Flare Poller [candidate] (4.303 ms) : 0, 4303
section profiling
crashtracking [baseline] (1.47 ms) : 0, 1470
crashtracking [candidate] (1.471 ms) : 0, 1471
BytebuddyAgent [baseline] (718.591 ms) : 0, 718591
BytebuddyAgent [candidate] (719.771 ms) : 0, 719771
GlobalTracer [baseline] (218.346 ms) : 0, 218346
GlobalTracer [candidate] (218.484 ms) : 0, 218484
AppSec [baseline] (32.279 ms) : 0, 32279
AppSec [candidate] (32.374 ms) : 0, 32374
Debugger [baseline] (6.711 ms) : 0, 6711
Debugger [candidate] (6.729 ms) : 0, 6729
Remote Config [baseline] (703.827 µs) : 0, 704
Remote Config [candidate] (695.326 µs) : 0, 695
Telemetry [baseline] (16.002 ms) : 0, 16002
Telemetry [candidate] (16.005 ms) : 0, 16005
Flare Poller [baseline] (4.062 ms) : 0, 4062
Flare Poller [candidate] (4.16 ms) : 0, 4160
ProfilingAgent [baseline] (110.022 ms) : 0, 110022
ProfilingAgent [candidate] (109.594 ms) : 0, 109594
Profiling [baseline] (110.664 ms) : 0, 110664
Profiling [candidate] (110.232 ms) : 0, 110232
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 3 performance regressions! Performance is the same for 8 metrics, 12 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.55.0-SNAPSHOT~7943bc960c, baseline=1.55.0-SNAPSHOT~60c4f68423
dateFormat X
axisFormat %s
section baseline
no_agent (4.153 ms) : 4098, 4209
. : milestone, 4153,
iast (10.458 ms) : 10281, 10635
. : milestone, 10458,
iast_FULL (14.709 ms) : 14414, 15004
. : milestone, 14709,
iast_GLOBAL (10.46 ms) : 10276, 10643
. : milestone, 10460,
profiling (8.513 ms) : 8377, 8650
. : milestone, 8513,
tracing (7.533 ms) : 7417, 7649
. : milestone, 7533,
section candidate
no_agent (4.316 ms) : 4266, 4366
. : milestone, 4316,
iast (10.125 ms) : 9940, 10309
. : milestone, 10125,
iast_FULL (14.685 ms) : 14386, 14983
. : milestone, 14685,
iast_GLOBAL (10.74 ms) : 10548, 10933
. : milestone, 10740,
profiling (8.419 ms) : 8293, 8544
. : milestone, 8419,
tracing (7.602 ms) : 7489, 7715
. : milestone, 7602,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.55.0-SNAPSHOT~7943bc960c, baseline=1.55.0-SNAPSHOT~60c4f68423
dateFormat X
axisFormat %s
section baseline
no_agent (37.596 ms) : 37295, 37897
. : milestone, 37596,
appsec (46.534 ms) : 46113, 46954
. : milestone, 46534,
code_origins (43.335 ms) : 42961, 43709
. : milestone, 43335,
iast (43.269 ms) : 42885, 43653
. : milestone, 43269,
profiling (50.107 ms) : 49632, 50582
. : milestone, 50107,
tracing (43.178 ms) : 42822, 43534
. : milestone, 43178,
section candidate
no_agent (36.238 ms) : 35952, 36524
. : milestone, 36238,
appsec (50.246 ms) : 49808, 50683
. : milestone, 50246,
code_origins (43.514 ms) : 43141, 43887
. : milestone, 43514,
iast (43.634 ms) : 43260, 44008
. : milestone, 43634,
profiling (50.569 ms) : 50073, 51064
. : milestone, 50569,
tracing (45.111 ms) : 44718, 45505
. : milestone, 45111,
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.55.0-SNAPSHOT~7943bc960c, baseline=1.55.0-SNAPSHOT~60c4f68423
dateFormat X
axisFormat %s
section baseline
no_agent (14.951 s) : 14951000, 14951000
. : milestone, 14951000,
appsec (14.888 s) : 14888000, 14888000
. : milestone, 14888000,
iast (18.679 s) : 18679000, 18679000
. : milestone, 18679000,
iast_GLOBAL (18.277 s) : 18277000, 18277000
. : milestone, 18277000,
profiling (15.456 s) : 15456000, 15456000
. : milestone, 15456000,
tracing (15.075 s) : 15075000, 15075000
. : milestone, 15075000,
section candidate
no_agent (14.815 s) : 14815000, 14815000
. : milestone, 14815000,
appsec (15.108 s) : 15108000, 15108000
. : milestone, 15108000,
iast (18.562 s) : 18562000, 18562000
. : milestone, 18562000,
iast_GLOBAL (17.924 s) : 17924000, 17924000
. : milestone, 17924000,
profiling (16.049 s) : 16049000, 16049000
. : milestone, 16049000,
tracing (14.819 s) : 14819000, 14819000
. : milestone, 14819000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.55.0-SNAPSHOT~7943bc960c, baseline=1.55.0-SNAPSHOT~60c4f68423
dateFormat X
axisFormat %s
section baseline
no_agent (1.468 ms) : 1457, 1479
. : milestone, 1468,
appsec (3.67 ms) : 3454, 3887
. : milestone, 3670,
iast (2.209 ms) : 2146, 2273
. : milestone, 2209,
iast_GLOBAL (2.245 ms) : 2181, 2309
. : milestone, 2245,
profiling (2.077 ms) : 2023, 2130
. : milestone, 2077,
tracing (2.021 ms) : 1971, 2070
. : milestone, 2021,
section candidate
no_agent (1.473 ms) : 1462, 1485
. : milestone, 1473,
appsec (2.445 ms) : 2394, 2496
. : milestone, 2445,
iast (2.2 ms) : 2136, 2263
. : milestone, 2200,
iast_GLOBAL (2.245 ms) : 2182, 2309
. : milestone, 2245,
profiling (2.077 ms) : 2023, 2130
. : milestone, 2077,
tracing (2.021 ms) : 1971, 2070
. : milestone, 2021,
|
PerfectSlayer
left a comment
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.
Sounds good but you will need to update WithSpan test suite to check for the new tag too.
…n and Trace Annotations (#9759) * re-working otel spec * fixing muzzle * fixing tests * updating tests * fixing test
What Does This Do
Currently, OTel Spans created from Trace Annotations do not contain the
span.kindtag, and spans created from custom instrumentation do not have thetypefield. This leads to an inconsistent customer experience where spans are missing data on the backend. This PR aims to reduce the inconsistent behavior between these two ways of creating OTel spans.This PR also adds support to translate
span.typetags set in OTel spans to set it as thetypefield of the Span itself.Motivation
Escalation
Additional Notes
RFC
Contributor Checklist
type:and (comp:orinst:) labels in addition to any useful labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]