Implement OpenTelemetry Logs API#11224
Implement OpenTelemetry Logs API#11224gh-worker-dd-mergequeue-cf854d[bot] merged 7 commits intomasterfrom
Conversation
ddd49d3 to
94d6f46
Compare
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 61 metrics, 10 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.62.0-SNAPSHOT~29949353c8, baseline=1.62.0-SNAPSHOT~e6cac64dfd
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.066 s) : 0, 1065577
Total [baseline] (11.025 s) : 0, 11025270
Agent [candidate] (1.062 s) : 0, 1061816
Total [candidate] (11.087 s) : 0, 11086598
section appsec
Agent [baseline] (1.267 s) : 0, 1266583
Total [baseline] (11.061 s) : 0, 11061105
Agent [candidate] (1.271 s) : 0, 1271207
Total [candidate] (11.044 s) : 0, 11044101
section iast
Agent [baseline] (1.249 s) : 0, 1249230
Total [baseline] (11.314 s) : 0, 11313970
Agent [candidate] (1.243 s) : 0, 1243101
Total [candidate] (11.36 s) : 0, 11359836
section profiling
Agent [baseline] (1.195 s) : 0, 1195129
Total [baseline] (11.06 s) : 0, 11059662
Agent [candidate] (1.185 s) : 0, 1185300
Total [candidate] (10.999 s) : 0, 10999346
gantt
title petclinic - break down per module: candidate=1.62.0-SNAPSHOT~29949353c8, baseline=1.62.0-SNAPSHOT~e6cac64dfd
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.227 ms) : 0, 1227
crashtracking [candidate] (1.228 ms) : 0, 1228
BytebuddyAgent [baseline] (636.703 ms) : 0, 636703
BytebuddyAgent [candidate] (634.585 ms) : 0, 634585
AgentMeter [baseline] (29.404 ms) : 0, 29404
AgentMeter [candidate] (29.658 ms) : 0, 29658
GlobalTracer [baseline] (248.903 ms) : 0, 248903
GlobalTracer [candidate] (248.74 ms) : 0, 248740
AppSec [baseline] (32.775 ms) : 0, 32775
AppSec [candidate] (32.741 ms) : 0, 32741
Debugger [baseline] (60.452 ms) : 0, 60452
Debugger [candidate] (59.923 ms) : 0, 59923
Remote Config [baseline] (610.834 µs) : 0, 611
Remote Config [candidate] (599.178 µs) : 0, 599
Telemetry [baseline] (9.862 ms) : 0, 9862
Telemetry [candidate] (9.336 ms) : 0, 9336
Flare Poller [baseline] (9.673 ms) : 0, 9673
Flare Poller [candidate] (9.074 ms) : 0, 9074
section appsec
crashtracking [baseline] (1.22 ms) : 0, 1220
crashtracking [candidate] (1.229 ms) : 0, 1229
BytebuddyAgent [baseline] (676.765 ms) : 0, 676765
BytebuddyAgent [candidate] (679.955 ms) : 0, 679955
AgentMeter [baseline] (12.278 ms) : 0, 12278
AgentMeter [candidate] (12.297 ms) : 0, 12297
GlobalTracer [baseline] (249.877 ms) : 0, 249877
GlobalTracer [candidate] (249.572 ms) : 0, 249572
IAST [baseline] (24.771 ms) : 0, 24771
IAST [candidate] (24.704 ms) : 0, 24704
AppSec [baseline] (185.472 ms) : 0, 185472
AppSec [candidate] (185.603 ms) : 0, 185603
Debugger [baseline] (64.749 ms) : 0, 64749
Debugger [candidate] (64.229 ms) : 0, 64229
Remote Config [baseline] (637.937 µs) : 0, 638
Remote Config [candidate] (558.393 µs) : 0, 558
Telemetry [baseline] (7.857 ms) : 0, 7857
Telemetry [candidate] (7.735 ms) : 0, 7735
Flare Poller [baseline] (6.195 ms) : 0, 6195
Flare Poller [candidate] (9.095 ms) : 0, 9095
section iast
crashtracking [baseline] (1.236 ms) : 0, 1236
crashtracking [candidate] (1.223 ms) : 0, 1223
BytebuddyAgent [baseline] (826.436 ms) : 0, 826436
BytebuddyAgent [candidate] (823.117 ms) : 0, 823117
AgentMeter [baseline] (11.341 ms) : 0, 11341
AgentMeter [candidate] (11.265 ms) : 0, 11265
GlobalTracer [baseline] (238.562 ms) : 0, 238562
GlobalTracer [candidate] (237.299 ms) : 0, 237299
IAST [baseline] (27.561 ms) : 0, 27561
IAST [candidate] (28.942 ms) : 0, 28942
AppSec [baseline] (32.322 ms) : 0, 32322
AppSec [candidate] (30.66 ms) : 0, 30660
Debugger [baseline] (63.578 ms) : 0, 63578
Debugger [candidate] (62.703 ms) : 0, 62703
Remote Config [baseline] (537.966 µs) : 0, 538
Remote Config [candidate] (513.514 µs) : 0, 514
Telemetry [baseline] (8.005 ms) : 0, 8005
Telemetry [candidate] (7.933 ms) : 0, 7933
Flare Poller [baseline] (3.461 ms) : 0, 3461
Flare Poller [candidate] (3.446 ms) : 0, 3446
section profiling
crashtracking [baseline] (1.195 ms) : 0, 1195
crashtracking [candidate] (1.174 ms) : 0, 1174
BytebuddyAgent [baseline] (697.616 ms) : 0, 697616
BytebuddyAgent [candidate] (691.854 ms) : 0, 691854
AgentMeter [baseline] (8.974 ms) : 0, 8974
AgentMeter [candidate] (8.877 ms) : 0, 8877
GlobalTracer [baseline] (209.578 ms) : 0, 209578
GlobalTracer [candidate] (207.487 ms) : 0, 207487
AppSec [baseline] (33.09 ms) : 0, 33090
AppSec [candidate] (32.655 ms) : 0, 32655
Debugger [baseline] (66.469 ms) : 0, 66469
Debugger [candidate] (65.672 ms) : 0, 65672
Remote Config [baseline] (586.689 µs) : 0, 587
Remote Config [candidate] (579.968 µs) : 0, 580
Telemetry [baseline] (8.161 ms) : 0, 8161
Telemetry [candidate] (8.045 ms) : 0, 8045
Flare Poller [baseline] (3.572 ms) : 0, 3572
Flare Poller [candidate] (3.538 ms) : 0, 3538
ProfilingAgent [baseline] (94.019 ms) : 0, 94019
ProfilingAgent [candidate] (93.996 ms) : 0, 93996
Profiling [baseline] (94.578 ms) : 0, 94578
Profiling [candidate] (94.548 ms) : 0, 94548
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.62.0-SNAPSHOT~29949353c8, baseline=1.62.0-SNAPSHOT~e6cac64dfd
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.062 s) : 0, 1061677
Total [baseline] (8.867 s) : 0, 8866750
Agent [candidate] (1.077 s) : 0, 1076671
Total [candidate] (8.863 s) : 0, 8862903
section iast
Agent [baseline] (1.246 s) : 0, 1245916
Total [baseline] (9.505 s) : 0, 9505450
Agent [candidate] (1.249 s) : 0, 1249363
Total [candidate] (9.559 s) : 0, 9558511
gantt
title insecure-bank - break down per module: candidate=1.62.0-SNAPSHOT~29949353c8, baseline=1.62.0-SNAPSHOT~e6cac64dfd
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.229 ms) : 0, 1229
crashtracking [candidate] (1.233 ms) : 0, 1233
BytebuddyAgent [baseline] (634.749 ms) : 0, 634749
BytebuddyAgent [candidate] (644.475 ms) : 0, 644475
AgentMeter [baseline] (29.279 ms) : 0, 29279
AgentMeter [candidate] (30.037 ms) : 0, 30037
GlobalTracer [baseline] (248.298 ms) : 0, 248298
GlobalTracer [candidate] (251.997 ms) : 0, 251997
AppSec [baseline] (32.655 ms) : 0, 32655
AppSec [candidate] (32.957 ms) : 0, 32957
Debugger [baseline] (59.891 ms) : 0, 59891
Debugger [candidate] (59.385 ms) : 0, 59385
Remote Config [baseline] (600.345 µs) : 0, 600
Remote Config [candidate] (604.658 µs) : 0, 605
Telemetry [baseline] (9.182 ms) : 0, 9182
Telemetry [candidate] (10.859 ms) : 0, 10859
Flare Poller [baseline] (9.853 ms) : 0, 9853
Flare Poller [candidate] (8.988 ms) : 0, 8988
section iast
crashtracking [baseline] (1.249 ms) : 0, 1249
crashtracking [candidate] (1.22 ms) : 0, 1220
BytebuddyAgent [baseline] (824.145 ms) : 0, 824145
BytebuddyAgent [candidate] (827.551 ms) : 0, 827551
AgentMeter [baseline] (11.282 ms) : 0, 11282
AgentMeter [candidate] (11.313 ms) : 0, 11313
GlobalTracer [baseline] (239.068 ms) : 0, 239068
GlobalTracer [candidate] (238.715 ms) : 0, 238715
AppSec [baseline] (31.77 ms) : 0, 31770
AppSec [candidate] (31.808 ms) : 0, 31808
Debugger [baseline] (62.994 ms) : 0, 62994
Debugger [candidate] (63.091 ms) : 0, 63091
Remote Config [baseline] (533.02 µs) : 0, 533
Remote Config [candidate] (524.122 µs) : 0, 524
Telemetry [baseline] (8.032 ms) : 0, 8032
Telemetry [candidate] (7.974 ms) : 0, 7974
Flare Poller [baseline] (3.358 ms) : 0, 3358
Flare Poller [candidate] (3.389 ms) : 0, 3389
IAST [baseline] (27.308 ms) : 0, 27308
IAST [candidate] (27.663 ms) : 0, 27663
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 1 performance regressions! Performance is the same for 14 metrics, 21 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~29949353c8, baseline=1.62.0-SNAPSHOT~e6cac64dfd
dateFormat X
axisFormat %s
section baseline
no_agent (18.217 ms) : 18031, 18402
. : milestone, 18217,
appsec (18.469 ms) : 18285, 18654
. : milestone, 18469,
code_origins (17.826 ms) : 17648, 18003
. : milestone, 17826,
iast (18.109 ms) : 17931, 18287
. : milestone, 18109,
profiling (18.456 ms) : 18272, 18641
. : milestone, 18456,
tracing (17.803 ms) : 17628, 17979
. : milestone, 17803,
section candidate
no_agent (19.251 ms) : 19056, 19447
. : milestone, 19251,
appsec (19.191 ms) : 18999, 19383
. : milestone, 19191,
code_origins (18.146 ms) : 17966, 18327
. : milestone, 18146,
iast (18.565 ms) : 18379, 18750
. : milestone, 18565,
profiling (18.4 ms) : 18220, 18581
. : milestone, 18400,
tracing (17.725 ms) : 17545, 17905
. : milestone, 17725,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~29949353c8, baseline=1.62.0-SNAPSHOT~e6cac64dfd
dateFormat X
axisFormat %s
section baseline
no_agent (1.269 ms) : 1255, 1283
. : milestone, 1269,
iast (3.315 ms) : 3270, 3359
. : milestone, 3315,
iast_FULL (6.037 ms) : 5977, 6098
. : milestone, 6037,
iast_GLOBAL (3.696 ms) : 3633, 3759
. : milestone, 3696,
profiling (2.258 ms) : 2236, 2281
. : milestone, 2258,
tracing (1.831 ms) : 1817, 1846
. : milestone, 1831,
section candidate
no_agent (1.25 ms) : 1238, 1263
. : milestone, 1250,
iast (3.222 ms) : 3178, 3265
. : milestone, 3222,
iast_FULL (6.077 ms) : 6015, 6138
. : milestone, 6077,
iast_GLOBAL (3.695 ms) : 3634, 3755
. : milestone, 3695,
profiling (2.285 ms) : 2262, 2307
. : milestone, 2285,
tracing (1.858 ms) : 1843, 1873
. : milestone, 1858,
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.62.0-SNAPSHOT~29949353c8, baseline=1.62.0-SNAPSHOT~e6cac64dfd
dateFormat X
axisFormat %s
section baseline
no_agent (15.045 s) : 15045000, 15045000
. : milestone, 15045000,
appsec (14.874 s) : 14874000, 14874000
. : milestone, 14874000,
iast (18.234 s) : 18234000, 18234000
. : milestone, 18234000,
iast_GLOBAL (17.674 s) : 17674000, 17674000
. : milestone, 17674000,
profiling (15.214 s) : 15214000, 15214000
. : milestone, 15214000,
tracing (14.884 s) : 14884000, 14884000
. : milestone, 14884000,
section candidate
no_agent (15.534 s) : 15534000, 15534000
. : milestone, 15534000,
appsec (14.638 s) : 14638000, 14638000
. : milestone, 14638000,
iast (18.736 s) : 18736000, 18736000
. : milestone, 18736000,
iast_GLOBAL (18.192 s) : 18192000, 18192000
. : milestone, 18192000,
profiling (14.77 s) : 14770000, 14770000
. : milestone, 14770000,
tracing (14.733 s) : 14733000, 14733000
. : milestone, 14733000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.62.0-SNAPSHOT~29949353c8, baseline=1.62.0-SNAPSHOT~e6cac64dfd
dateFormat X
axisFormat %s
section baseline
no_agent (1.5 ms) : 1488, 1512
. : milestone, 1500,
appsec (3.826 ms) : 3602, 4050
. : milestone, 3826,
iast (2.29 ms) : 2220, 2360
. : milestone, 2290,
iast_GLOBAL (2.324 ms) : 2254, 2395
. : milestone, 2324,
profiling (2.111 ms) : 2056, 2166
. : milestone, 2111,
tracing (2.088 ms) : 2034, 2142
. : milestone, 2088,
section candidate
no_agent (1.493 ms) : 1482, 1505
. : milestone, 1493,
appsec (2.567 ms) : 2511, 2622
. : milestone, 2567,
iast (2.291 ms) : 2221, 2361
. : milestone, 2291,
iast_GLOBAL (2.337 ms) : 2267, 2407
. : milestone, 2337,
profiling (2.109 ms) : 2054, 2163
. : milestone, 2109,
tracing (2.084 ms) : 2030, 2137
. : milestone, 2084,
|
f7be2bf to
a078088
Compare
a078088 to
b15ea59
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b15ea597e4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Nice to see some JUnit instrumentation testing 😉
I'm submitting partial review, for the instrumentation only as I see some changes being pushed in parallel. I'm continuing the review of api / core this morning.
Update: The second part looks good, not much feedback to give about it 👍
| public final int bodyType; | ||
| @Nullable public final Object bodyValue; | ||
| @Nullable public final String eventName; | ||
| @Nullable public final Map<?, ?> attributes; |
There was a problem hiding this comment.
💭 thought: Null map (or collection) can be counter intuitive. What about having it non-null and use static empty collection when needed? It feels more consistent with the codebase.
| import javax.annotation.Nullable; | ||
| import javax.annotation.ParametersAreNonnullByDefault; | ||
|
|
||
| @ParametersAreNonnullByDefault |
There was a problem hiding this comment.
🎯 suggestion: What about having it as package annotation? It would avoid having to use it in all classes and keep any new class consistent too?
There was a problem hiding this comment.
It's tempting, but I was worried that was less visible to developers - if I make that change I'll do it in a separate PR as the other existing sub-packages here follow the convention of putting it on the class
There was a problem hiding this comment.
There is pros and cons. I think the visibility part come from the fact we don't use it enough, even for documentation. But it will save us for case when devs will definitely forget it like inner classes.
Sure, you can review it any time later, or not! That was only a suggestion :)
| bodyType, | ||
| bodyValue, | ||
| eventName, | ||
| attributes != null ? new HashMap<>(attributes) : null, |
There was a problem hiding this comment.
❔ question: Do we have to create a new HashMap? Is it for defensive copy?
I could only find call to empty() and forEach() calls later down record lifecycle on your side.
If it's to protect against build reuse and attribute modification after the log is "emitted", OTel current implementation does not protect for it.
So Wouldn't an unmodiafiableMap() wrapper be enough? I'm asking wrt to the cost of new HashMap related allocation.
There was a problem hiding this comment.
So it is defensive - to stop situations where the caller goes on to modify attributes via the builder after calling emit. Using unmodifiableMap wouldn't help because that just stops the map from being modified through that wrapper. The builder would still have write access to the backing map.
However there's a better way to do this - pass the original map to through emit (avoiding allocations) but add a flag so if the user continued to use the builder and called setAttribute then we copy the map at that point. This way we'd protect against unexpected use, while still avoid allocations in the happy case.
There was a problem hiding this comment.
Using unmodifiableMap wouldn't help because that just stops the map from being modified through that wrapper. The builder would still have write access to the backing map.
Sorry, that was from my original comment where I was looking down stream use. But I figure out about side modifications from the builder, that where I amend my comment about OTel implementation not protecting against it either 😅
You're solution is indeed better, despite not following OTel behavior -- that could be something to upstream 😇
| implements Instrumenter.CanShortcutTypeMatching, Instrumenter.HasMethodAdvice { | ||
|
|
||
| public OpenTelemetryLogsInstrumentation() { | ||
| super("opentelemetry-logs", "opentelemetry-1.27"); |
There was a problem hiding this comment.
💭 thought: What about having a common alias for all instrumentations related to OTel? Like tracing use opentelemetry-1. Could it be useful here too?
| import io.opentelemetry.api.logs.Logger; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| abstract class OpenTelemetry127ActivationTest extends AbstractInstrumentationTest { |
There was a problem hiding this comment.
❔ question: Any reason the activation tests won't go under a Java package?
There was a problem hiding this comment.
No reason other than following the same layout as the older groovy tests - I'll move them
Also make attributes non-null in OtlpLogRecord.
PerfectSlayer
left a comment
There was a problem hiding this comment.
Approving after the first round of comments. Thanks for the follow up changes! 👍
…match OTel traces instrumentation
f25ecef to
0ac061d
Compare
| for (int i = 0; i < batchSize; i++) { | ||
| OtlpLogRecord logRecord = queue.poll(); | ||
| if (logRecord != null) { | ||
| batch.add(logRecord); | ||
| } else { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Nit: would it be more human-readable to use something like:
OtlpLogRecord logRecord;
while ((logRecord = queue.poll()) != null) {
batch.add(logRecord);
}
WDYT?
There was a problem hiding this comment.
That would continually grow the batch if another thread is continually adding log records, potentially delaying the export indefinitely. That's why we grab the number of pending records first and then drain only those. Any records added after that initial sizing will be handled by the next scheduled export.
There was a problem hiding this comment.
Got it! probably make sense to add a comment for future generations? :)
| def openTelemetryVersion = '1.27.0' | ||
|
|
||
| muzzle { | ||
| pass { | ||
| module = 'opentelemetry-api' |
There was a problem hiding this comment.
Nit: Consider to use Kotlin (if possible).
Nit: Consider to generate lock file.
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What Does This Do
Implements the OpenTelemetry Logs API (since OpenTelemetry 1.27) along with a batched collector.
This will be used by an upcoming PR to export OTel logs over OTLP.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.