Support addEvent Otel API #7408
Merged
+398
−35
Conversation
This file contains 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
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 15 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.40.0-SNAPSHOT~3af7937d33, baseline=1.40.0-SNAPSHOT~c59beb4dc9
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.568 s) : 0, 1568370
Total [baseline] (14.158 s) : 0, 14158048
Agent [candidate] (1.565 s) : 0, 1565333
Total [candidate] (14.302 s) : 0, 14302435
section appsec
Agent [baseline] (1.77 s) : 0, 1769793
Total [baseline] (14.518 s) : 0, 14518005
Agent [candidate] (1.759 s) : 0, 1759228
Total [candidate] (14.548 s) : 0, 14548298
section iast
Agent [baseline] (1.735 s) : 0, 1735499
Total [baseline] (14.904 s) : 0, 14904290
Agent [candidate] (1.731 s) : 0, 1730702
Total [candidate] (14.733 s) : 0, 14732580
section profiling
Agent [baseline] (1.876 s) : 0, 1876310
Total [baseline] (14.465 s) : 0, 14465425
Agent [candidate] (1.874 s) : 0, 1873830
Total [candidate] (14.482 s) : 0, 14482200
gantt
title petclinic - break down per module: candidate=1.40.0-SNAPSHOT~3af7937d33, baseline=1.40.0-SNAPSHOT~c59beb4dc9
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (1.005 s) : 0, 1004598
BytebuddyAgent [candidate] (1.003 s) : 0, 1002978
GlobalTracer [baseline] (460.068 ms) : 0, 460068
GlobalTracer [candidate] (458.869 ms) : 0, 458869
AppSec [baseline] (73.018 ms) : 0, 73018
AppSec [candidate] (72.731 ms) : 0, 72731
Remote Config [baseline] (819.846 µs) : 0, 820
Remote Config [candidate] (858.958 µs) : 0, 859
Telemetry [baseline] (9.721 ms) : 0, 9721
Telemetry [candidate] (9.699 ms) : 0, 9699
section appsec
BytebuddyAgent [baseline] (1.036 s) : 0, 1036478
BytebuddyAgent [candidate] (1.028 s) : 0, 1027912
GlobalTracer [baseline] (449.069 ms) : 0, 449069
GlobalTracer [candidate] (447.954 ms) : 0, 447954
AppSec [baseline] (239.175 ms) : 0, 239175
AppSec [candidate] (237.955 ms) : 0, 237955
IAST [baseline] (26.266 ms) : 0, 26266
IAST [candidate] (25.745 ms) : 0, 25745
Remote Config [baseline] (788.909 µs) : 0, 789
Remote Config [candidate] (791.171 µs) : 0, 791
Telemetry [baseline] (11.011 ms) : 0, 11011
Telemetry [candidate] (11.605 ms) : 0, 11605
section iast
BytebuddyAgent [baseline] (1.165 s) : 0, 1164932
BytebuddyAgent [candidate] (1.16 s) : 0, 1160386
GlobalTracer [baseline] (437.435 ms) : 0, 437435
GlobalTracer [candidate] (437.416 ms) : 0, 437416
AppSec [baseline] (72.561 ms) : 0, 72561
AppSec [candidate] (70.49 ms) : 0, 70490
IAST [baseline] (30.003 ms) : 0, 30003
IAST [candidate] (30.068 ms) : 0, 30068
Remote Config [baseline] (837.517 µs) : 0, 838
Remote Config [candidate] (2.45 ms) : 0, 2450
Telemetry [baseline] (9.453 ms) : 0, 9453
Telemetry [candidate] (9.614 ms) : 0, 9614
section profiling
BytebuddyAgent [baseline] (992.295 ms) : 0, 992295
BytebuddyAgent [candidate] (991.386 ms) : 0, 991386
GlobalTracer [baseline] (584.686 ms) : 0, 584686
GlobalTracer [candidate] (585.282 ms) : 0, 585282
AppSec [baseline] (73.599 ms) : 0, 73599
AppSec [candidate] (73.095 ms) : 0, 73095
Remote Config [baseline] (836.788 µs) : 0, 837
Remote Config [candidate] (851.029 µs) : 0, 851
Telemetry [baseline] (9.458 ms) : 0, 9458
Telemetry [candidate] (9.478 ms) : 0, 9478
ProfilingAgent [baseline] (158.892 ms) : 0, 158892
ProfilingAgent [candidate] (157.239 ms) : 0, 157239
Profiling [baseline] (158.952 ms) : 0, 158952
Profiling [candidate] (157.301 ms) : 0, 157301
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.40.0-SNAPSHOT~3af7937d33, baseline=1.40.0-SNAPSHOT~c59beb4dc9
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.571 s) : 0, 1571088
Total [baseline] (11.818 s) : 0, 11818104
Agent [candidate] (1.561 s) : 0, 1560828
Total [candidate] (11.769 s) : 0, 11768625
section iast
Agent [baseline] (1.731 s) : 0, 1730862
Total [baseline] (12.484 s) : 0, 12483505
Agent [candidate] (1.727 s) : 0, 1726993
Total [candidate] (12.513 s) : 0, 12512935
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.733 s) : 0, 1732960
Total [baseline] (12.412 s) : 0, 12412480
Agent [candidate] (1.725 s) : 0, 1725069
Total [candidate] (12.434 s) : 0, 12434499
section iast_TELEMETRY_OFF
Agent [baseline] (1.73 s) : 0, 1730180
Total [baseline] (12.446 s) : 0, 12445583
Agent [candidate] (1.72 s) : 0, 1719603
Total [candidate] (12.426 s) : 0, 12426082
gantt
title insecure-bank - break down per module: candidate=1.40.0-SNAPSHOT~3af7937d33, baseline=1.40.0-SNAPSHOT~c59beb4dc9
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (1.005 s) : 0, 1005445
BytebuddyAgent [candidate] (1.001 s) : 0, 1000649
GlobalTracer [baseline] (461.681 ms) : 0, 461681
GlobalTracer [candidate] (457.073 ms) : 0, 457073
AppSec [baseline] (73.293 ms) : 0, 73293
AppSec [candidate] (72.415 ms) : 0, 72415
Remote Config [baseline] (829.0 µs) : 0, 829
Remote Config [candidate] (846.197 µs) : 0, 846
Telemetry [baseline] (9.627 ms) : 0, 9627
Telemetry [candidate] (9.661 ms) : 0, 9661
section iast
BytebuddyAgent [baseline] (1.163 s) : 0, 1163143
BytebuddyAgent [candidate] (1.158 s) : 0, 1158008
GlobalTracer [baseline] (435.501 ms) : 0, 435501
GlobalTracer [candidate] (436.262 ms) : 0, 436262
AppSec [baseline] (71.888 ms) : 0, 71888
AppSec [candidate] (71.438 ms) : 0, 71438
Remote Config [baseline] (761.962 µs) : 0, 762
Remote Config [candidate] (2.553 ms) : 0, 2553
Telemetry [baseline] (9.376 ms) : 0, 9376
Telemetry [candidate] (9.493 ms) : 0, 9493
IAST [baseline] (29.923 ms) : 0, 29923
IAST [candidate] (28.952 ms) : 0, 28952
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (1.163 s) : 0, 1163271
BytebuddyAgent [candidate] (1.155 s) : 0, 1155466
GlobalTracer [baseline] (437.316 ms) : 0, 437316
GlobalTracer [candidate] (436.362 ms) : 0, 436362
AppSec [baseline] (70.901 ms) : 0, 70901
AppSec [candidate] (71.518 ms) : 0, 71518
Remote Config [baseline] (843.538 µs) : 0, 844
Remote Config [candidate] (799.882 µs) : 0, 800
Telemetry [baseline] (9.457 ms) : 0, 9457
Telemetry [candidate] (12.129 ms) : 0, 12129
IAST [baseline] (30.969 ms) : 0, 30969
IAST [candidate] (28.576 ms) : 0, 28576
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (1.16 s) : 0, 1160381
BytebuddyAgent [candidate] (1.151 s) : 0, 1151365
GlobalTracer [baseline] (437.364 ms) : 0, 437364
GlobalTracer [candidate] (436.064 ms) : 0, 436064
AppSec [baseline] (71.696 ms) : 0, 71696
AppSec [candidate] (69.657 ms) : 0, 69657
Remote Config [baseline] (756.874 µs) : 0, 757
Remote Config [candidate] (771.588 µs) : 0, 772
Telemetry [baseline] (10.971 ms) : 0, 10971
Telemetry [candidate] (12.656 ms) : 0, 12656
IAST [baseline] (28.765 ms) : 0, 28765
IAST [candidate] (28.859 ms) : 0, 28859
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 16 unstable metrics. Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.40.0-SNAPSHOT~3af7937d33, baseline=1.40.0-SNAPSHOT~c59beb4dc9
dateFormat X
axisFormat %s
section baseline
no_agent (367.974 µs) : 348, 388
. : milestone, 368,
iast (482.824 µs) : 462, 504
. : milestone, 483,
iast_FULL (553.993 µs) : 533, 575
. : milestone, 554,
iast_GLOBAL (497.331 µs) : 476, 518
. : milestone, 497,
iast_HARDCODED_SECRET_DISABLED (482.027 µs) : 461, 503
. : milestone, 482,
iast_INACTIVE (442.523 µs) : 422, 463
. : milestone, 443,
iast_TELEMETRY_OFF (476.151 µs) : 454, 499
. : milestone, 476,
tracing (435.373 µs) : 415, 455
. : milestone, 435,
section candidate
no_agent (371.976 µs) : 353, 391
. : milestone, 372,
iast (486.118 µs) : 464, 508
. : milestone, 486,
iast_FULL (556.696 µs) : 536, 578
. : milestone, 557,
iast_GLOBAL (508.242 µs) : 487, 530
. : milestone, 508,
iast_HARDCODED_SECRET_DISABLED (485.787 µs) : 463, 508
. : milestone, 486,
iast_INACTIVE (448.341 µs) : 428, 469
. : milestone, 448,
iast_TELEMETRY_OFF (466.763 µs) : 444, 489
. : milestone, 467,
tracing (444.896 µs) : 425, 465
. : milestone, 445,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.40.0-SNAPSHOT~3af7937d33, baseline=1.40.0-SNAPSHOT~c59beb4dc9
dateFormat X
axisFormat %s
section baseline
no_agent (1.35 ms) : 1331, 1369
. : milestone, 1350,
appsec (1.706 ms) : 1681, 1731
. : milestone, 1706,
appsec_no_iast (1.711 ms) : 1687, 1735
. : milestone, 1711,
iast (1.483 ms) : 1460, 1506
. : milestone, 1483,
profiling (1.48 ms) : 1457, 1503
. : milestone, 1480,
tracing (1.459 ms) : 1433, 1484
. : milestone, 1459,
section candidate
no_agent (1.355 ms) : 1335, 1374
. : milestone, 1355,
appsec (1.738 ms) : 1714, 1761
. : milestone, 1738,
appsec_no_iast (1.731 ms) : 1708, 1755
. : milestone, 1731,
iast (1.491 ms) : 1468, 1515
. : milestone, 1491,
profiling (1.507 ms) : 1483, 1531
. : milestone, 1507,
tracing (1.474 ms) : 1450, 1498
. : milestone, 1474,
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.40.0-SNAPSHOT~3af7937d33, baseline=1.40.0-SNAPSHOT~c59beb4dc9
dateFormat X
axisFormat %s
section baseline
no_agent (1.459 ms) : 1448, 1470
. : milestone, 1459,
appsec (2.282 ms) : 2242, 2323
. : milestone, 2282,
iast (2.037 ms) : 1988, 2086
. : milestone, 2037,
iast_GLOBAL (2.096 ms) : 2045, 2148
. : milestone, 2096,
profiling (1.917 ms) : 1877, 1957
. : milestone, 1917,
tracing (1.896 ms) : 1858, 1935
. : milestone, 1896,
section candidate
no_agent (1.459 ms) : 1448, 1471
. : milestone, 1459,
appsec (2.285 ms) : 2244, 2326
. : milestone, 2285,
iast (2.024 ms) : 1976, 2073
. : milestone, 2024,
iast_GLOBAL (2.091 ms) : 2040, 2141
. : milestone, 2091,
profiling (1.906 ms) : 1865, 1946
. : milestone, 1906,
tracing (1.898 ms) : 1859, 1936
. : milestone, 1898,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.40.0-SNAPSHOT~3af7937d33, baseline=1.40.0-SNAPSHOT~c59beb4dc9
dateFormat X
axisFormat %s
section baseline
no_agent (15.025 s) : 15025000, 15025000
. : milestone, 15025000,
appsec (15.205 s) : 15205000, 15205000
. : milestone, 15205000,
iast (18.762 s) : 18762000, 18762000
. : milestone, 18762000,
iast_GLOBAL (17.924 s) : 17924000, 17924000
. : milestone, 17924000,
profiling (15.506 s) : 15506000, 15506000
. : milestone, 15506000,
tracing (14.861 s) : 14861000, 14861000
. : milestone, 14861000,
section candidate
no_agent (14.825 s) : 14825000, 14825000
. : milestone, 14825000,
appsec (15.196 s) : 15196000, 15196000
. : milestone, 15196000,
iast (19.129 s) : 19129000, 19129000
. : milestone, 19129000,
iast_GLOBAL (18.109 s) : 18109000, 18109000
. : milestone, 18109000,
profiling (15.071 s) : 15071000, 15071000
. : milestone, 15071000,
tracing (15.189 s) : 15189000, 15189000
. : milestone, 15189000,
|
...agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java
Outdated
Show resolved
Hide resolved
Comment on lines
33
to
53
| public String toString() { | ||
| // TODO if attributes exist, process those | ||
| return "{\"name\":\"" | ||
| + this.name | ||
| + "\",\"time_unix_nano\":" | ||
| + Long.toString(this.timestamp) | ||
| + "}"; | ||
| } | ||
|
|
||
| // @NotNull | ||
| // TODO: give this a notnull annotation | ||
| public static String toTag(List<OtelSpanEvent> events) { | ||
| StringBuilder builder = new StringBuilder("["); | ||
| for (int i = 0; i < events.size(); i++) { | ||
| if (i > 0) { | ||
| builder.append(","); | ||
| } | ||
| builder.append(events.get(i).toString()); | ||
| } | ||
| return builder.append("]").toString(); | ||
| } |
There was a problem hiding this comment.
I haven't check at the RFC yet but this looks like JSON.
Rather than try to avoid any conversion issue, like escaping double quote from attribute name, what about reusing the JsonBuffer that @dougqh is building in bootstrap?
Or maybe simply use Moshi: implementation libs.moshi
There was a problem hiding this comment.
@PerfectSlayer unfortunately we can't use moshi directly because of where this code ends up in the final jar (and how it gets injected into the user's application) - given the JSON output is straightforward I'd suggest manually encoding it, as before, or re-using the JsonBuffer from Doug's PR (we'd need to decide what's the best place to host that code)
There was a problem hiding this comment.
Yeah, that’s what I though and why I propose Doug’s code in the first place.
We can find a proper place and add some tests to promote it as a resuable piece of code 👍
There was a problem hiding this comment.
@PerfectSlayer , Stuart and I discussed just using the manual parsing method I had initially, and to migrate the code to use Doug's parser once that's been merged (so as not to block this PR)
There was a problem hiding this comment.
Right now, my JsonBuffer is only in the bootstrap, but we could make it available elsewhere, too.
I'm also open to modifying it to having a template mode, so we can prebuild most of the structure and reuse.
It also already has a mechanism to put multiple buffers together if that would be of use here.
mtoffl01
commented
Aug 12, 2024
mcculls
reviewed
Aug 12, 2024
...ava-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java
Outdated
Show resolved
Hide resolved
mcculls
reviewed
Aug 12, 2024
| StringBuilder builder = new StringBuilder("["); | ||
| int index = 0; | ||
| while (index < events.size()) { | ||
| String linkAsJson = getEncoder().toJson(events.get(index)); |
There was a problem hiding this comment.
As mentioned above we can avoid the moshi dependency if we wrote out own method(s) to write span event data as JSON. The structure is simple, so this shouldn't involve much code. Also we only need to handle writing JSON, not parsing it.
There was a problem hiding this comment.
One question I have is what the RFC states about encoding when reaching tag max length?
Should the tracer take care of it? Should we limit event count or drop attributes?
Because if the tag value is cropped, I don’t know what will happen on the backend side with invalid JSON object 🤷
There was a problem hiding this comment.
The RFC does not specify any limits, however:
- Otel Span Events API does not specify any limits on span event count
- However, Otel does enforce a [default limit of 128 span events per span and a maximum of 128 attributes per span event](Span Events, they include a default of 128 maximum span events per span and a maximum of 128 attributes per span event) within the SDK
- Our implementation of Span Links enforces a character limit with
TAG_MAX_LENGTH
… attributes if conflict
mtoffl01
commented
Aug 15, 2024
...ent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java
Outdated
Show resolved
Hide resolved
mtoffl01
commented
Aug 15, 2024
...ava-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java
Outdated
Show resolved
Hide resolved
mtoffl01
commented
Aug 15, 2024
...t/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy
Outdated
Show resolved
Hide resolved
…taing stringification for span link attributes
mtoffl01
commented
Aug 16, 2024
...t/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy
Outdated
Show resolved
Hide resolved
7 tasks
mtoffl01
commented
Aug 26, 2024
...ent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java
Outdated
Show resolved
Hide resolved
amarziali
reviewed
Aug 27, 2024
internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java
Outdated
Show resolved
Hide resolved
|
Note: Test coverage is failing for SpanAttributes.Builder and SpanAttributes.JSONParser, but I'm confused by the results. The Opentelemetry14Test tests for adding events do utilize the builder and JSONParser, and when I run a test with coverage, we can see green on |
There was a problem hiding this comment.
I mostly focused on the OTel part and left a bunch of comments (sorry for that 😓 ).
In addition, I don't think we should reuse the span attributes for the event attributes.
Encoding and handling are not the same, and the introduced format impact nearly every single behavior (all except than String handling).
As span events are not a thing outside OpenTelemetry, they should not impact the Span.Attributes class nor the core part. I would advocate for creating a SpanEvent.Attribute class to handle this specifically for OTel and revert any change to internal-api and core.
How do you feel about it?
...ava-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java
Outdated
Show resolved
Hide resolved
dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpan.java
Outdated
Show resolved
Hide resolved
dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpan.java
Outdated
Show resolved
Hide resolved
dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpan.java
Outdated
Show resolved
Hide resolved
...ava-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java
Outdated
Show resolved
Hide resolved
...ava-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java
Outdated
Show resolved
Hide resolved
...ava-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java
Outdated
Show resolved
Hide resolved
...ent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java
Outdated
Show resolved
Hide resolved
...ent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java
Outdated
Show resolved
Hide resolved
...ent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java
Outdated
Show resolved
Hide resolved
7 tasks
Move convention related span event handling to OtelConventions Move span event related logic to OtelSpanEvent Add OTel attributes cache for performance Add comment about nullable field Rename OtelSpanEvent from toString to toJson to avoid confusion with Java Object semantic Add OtelSpanEvent toString method for debugging Add OtelSpanEvent recordException test cases Improve JSON encoding to use buffer length and char append when possible Rename AttributeJsonParser to follow Java naming conventions Clean up Javadoc
Add test to check events behavior after span end Simplify expected JSON building Remove custom time source when not needed Used tag name from constants Ensure span close Improve time API usage Fix expected JSON keys
PerfectSlayer
approved these changes
Sep 16, 2024
PerfectSlayer
added
type: enhancement
inst: opentelemetry
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
Adds support for span events via
Span.addEventandSpan.recordException.Motivation
https://datadoghq.atlassian.net/browse/APMAPI-199
Additional Notes
Passing all system tests enabled here: DataDog/system-tests#3027
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]