⭐ Football Update: aws observability/aws otel java instrumentation/pull/

srprash · 2026-06-03T23:12:50Z

Description

Adds Dynamic Instrumentation: the ability to capture additional runtime telemetry (snapshots from probes and breakpoints) from a running application without a restart or redeploy. The agent periodically polls instrumentation configurations, applies them at runtime via bytecode instrumentation, captures the requested runtime context as snapshots, and exports them as OTLP log records.

The feature is opt-in and disabled by default (OTEL_AWS_DYNAMIC_INSTRUMENTATION_ENABLED, default false). When disabled, no polling threads start, no configurations are fetched, and no bytecode is transformed — there is no change to application behavior. It is also automatically skipped on AWS Lambda.

See docs/dynamic-instrumentation.md for configuration and usage.

What's included

The change is organized into two commits for review:

Bootstrap bridge + agent entry-point scaffolding (inert on its own):
- New di-bootstrap-bridge module — dependency-free classes loaded via the bootstrap classloader for cross-classloader data sharing between instrumented application code and the agent.
- AwsInstrumentationHolder — holds the Instrumentation reference for runtime retransformation. setInstrumentation is idempotent and never throws, since it sits on the universal agent entry path and must not be able to abort JVM startup (e.g. on a duplicate -javaagent).
- AwsAgentBootstrap.premain appends the agent JAR to the bootstrap classloader search (so the bridge classes embedded at the JAR root are visible to all classloaders) and stores the Instrumentation reference. The append is wrapped so a failure can never abort agent startup.
- Build wiring to include and embed the bridge module.
Dynamic Instrumentation feature (opt-in, default off):
- Configuration polling client, ByteBuddy/ASM instrumentation engine, capture models, snapshot collection, and OTLP-logs emitter; registered via the autoconfigure SPI.
- Setup failures are isolated so they cannot affect the rest of the agent or the application.
- okhttp and ASM dependencies added (ASM relocated).

Safety

Off unless explicitly enabled; the disabled path is a no-op (verified: no DI threads, no overhead).
A Dynamic Instrumentation setup failure is caught and isolated so it cannot disable the rest of the agent's telemetry or the application.
Captured values are bounded by enforced maximums (string length, collection width).

Testing

Unit tests: 16 test classes covering the DI subsystems (polling/client, config, instrumentation engine and registry, capture models and rate limiting, snapshot serialization, OTLP emitter).
Contract tests: new diContractTests Gradle task with a di-spring-boot test image and an OTLP-logs mock collector, exercising the full path (agent → bytecode instrumentation → snapshot capture → OTLP logs → validation) across breakpoints, line breakpoints, probes, probe+breakpoint coexistence, max-hits enforcement, and capture limits.
CI: new di-contract-tests workflow (Java 17 and 21).
Verified the whole-repo build is green with no regression, and ran manual DI-off (no-op) and DI-on smoke tests.

Notes for reviewers

The two DICaptureLimitsTest contract tests are timing-sensitive and can be flaky on slower runners; the underlying behavior (capture-limit clamping) is exercised and passes when timing is favorable. A separate hardening of the test's snapshot-matching is worth a follow-up.

Add the cross-classloader plumbing that Dynamic Instrumentation builds on, isolated from the feature itself so it can be reviewed independently. This change is inert on its own: no DI behavior is enabled. - New di-bootstrap-bridge module: dependency-free classes loaded via the bootstrap classloader for cross-classloader data sharing between instrumented application code and the agent. - AwsInstrumentationHolder: holds the Instrumentation reference for runtime retransformation. setInstrumentation is idempotent and never throws, since it sits on the universal agent entry path and must not be able to abort JVM startup (e.g. on a duplicate -javaagent). - AwsAgentBootstrap.premain now appends the agent JAR to the bootstrap classloader search (so the bridge classes embedded at the JAR root are visible to all classloaders) and stores the Instrumentation reference. The append is wrapped so a failure can never abort agent startup. - Build wiring: include the bridge module, embed its classes at the agent JAR root, exclude it from dependency management (must stay dependency-free).

github-actions

PR Review: reviewed the core DI implementation. Posting inline comments on specific issues.

github-actions

Additional code quality observations.

github-actions

PR Review: Dynamic Instrumentation - Findings Summary

HIGH SEVERITY:

Security - ReflectiveValueSerializer.java:82 - field.setAccessible(true) on all fields with no denylist for sensitive types/field names. Secrets can be exfiltrated.
Bug - DIDataStore.java:35 - Unbounded ConcurrentLinkedQueue causes OOM under load.

MEDIUM SEVERITY:
3. Bug - DIDataStore.java:297 - Rate limiter race on window rollover.
4. Bug - DynamicInstrumentationClient.java:137 - Unbounded response body read.
5. Security - DynamicInstrumentationClient.java:303 - Fail-open on filter exception.
6. Bug - AwsDynamicInstrumentationCustomizerProvider.java:148 - Non-daemon thread prevents shutdown.
7. Perf - ByteBuddyInstrumentationEngine.java:380 - O(n*m) getAllLoadedClasses scan.
8. Bug - StatusReporter.java:60 - reportedConfigs memory leak.
9. Bug - DIDataStore.java:41 - ThreadLocal leak on missed exit.
10. Perf - DIDataStore.java:83 - 100ms serialization on app thread.

LOW SEVERITY:
11. Quality - AwsDynamicInstrumentationCustomizerProvider.java:220 - Redundant e.printStackTrace().

DIDataStore held pending method-entry data in a ThreadLocal map keyed by method name. For a recursive or re-entrant method, every frame shares the same key, so each entry overwrote the previous frame's data and only the last survived. The first exit consumed it and later exits emitted method snapshots with no arguments (e.g. recursiveSum(4) -> 4 snapshots, only 1 with the captured arg). Store pending entries as a per-key LIFO stack (ThreadLocal<Map<String, Deque<PendingEntryData>>>): entry pushes a frame, exit pops the innermost matching frame, so each frame's arguments survive to its own exit. Empty stacks are pruned to avoid ThreadLocal growth. The existing exit-only fallback is preserved (a frame whose entry capture was skipped still produces a snapshot). Bootstrap module stays dependency-free. Add DIDataStoreTest coverage simulating recursive entry/exit and asserting each capture carries its own frame's arguments.

Reflow comments and statements that exceeded the 100-column limit so spotlessCheck passes in CI. Formatting only; no behavior change.

Address concurrency review findings in the DI init/poll path: - ConfigurationPoller: replace the volatile boolean `running` with an AtomicBoolean and use compareAndSet in start()/stop(). The previous check-then-act could let two concurrent start() callers both create polling threads. Add a concurrency test asserting a start storm creates the poller threads only once. - DynamicInstrumentationManager: mark the instance fields (config, client, engine, statusReporter, collector, otlpEmitter, ...) volatile so writes from initialize() are visible to the poller/shutdown threads that read them. Guard applyConfigurations()/removeInstrumentations() with an initialized.get() fence so they no-op before init or after shutdown, and flip initialized=false before cleanup() so those methods stop operating before fields are nulled (prevents a shutdown-races-with-apply NPE). Add tests for the pre-initialize no-op behavior. - AwsDynamicInstrumentationCustomizerProvider: correct a misleading comment on the 100ms init delay — it is not a synchronization mechanism; the manager intentionally uses TracerProvider.noop() and does not depend on the global TracerProvider being ready.

github-actions

Code Review

Reviewed this large PR (15.5k additions) for bugs, security issues, and code quality. Overall this is a well-structured implementation with good defensive coding patterns (no-fail error handling in agent paths, limit clamping, isolated subsystem failures). The two-commit structure (bootstrap bridge, then DI feature) is clear and reviewable.

Found 2 security concerns, 3 potential bugs, and 2 code quality issues detailed in the inline comments below.

vastin

LGTM

…y test (#1388) ## Description The [main build](https://github.com/aws-observability/aws-otel-java-instrumentation/actions/runs/27241134973) failed intermittently after #1384 on: ``` ConfigurationPollerTest > concurrentStartCreatesPollerThreadsOnlyOnce() FAILED expected: 1 but was: 0 ``` ### Root cause `ConfigurationPoller.stop()` flipped the `running` flag but never interrupted the poller threads, leaving them asleep in their poll-interval backoff for up to ~15s after `stop()` returned. As a result: - `stop()` always blocked the full 10s latch timeout (logging "Polling threads did not stop within timeout"). - Each test calling `stop()` leaked `ProbePoller`/`BreakpointPoller` threads that died asynchronously. - In `concurrentStartCreatesPollerThreadsOnlyOnce`, a leaked thread from an earlier test dying between the baseline thread count and the assertion offset the delta to 0 — the exact CI failure. The test is timing-dependent, which is why a re-run passed. ### Changes - `stop()` now interrupts both poller threads before awaiting the stop latch. The polling loops already catch `InterruptedException` and break cleanly, so threads exit immediately. This also makes shutdown deterministic in production instead of always eating the 10s timeout. - `start()`/`stop()` are now `synchronized` so a `stop()` racing a `start()` always observes fully initialized thread references and stop latch (these are plain fields with no other happens-before edge). - The concurrency test waits for leftover poller threads to exit before taking its baseline counts, closing the flake window entirely. ## Testing - Full `:awsagentprovider` suite: 713 tests, 0 failures (same count as CI). - `spotlessCheck` passes. - `ConfigurationPollerTest` runtime drops from ~54s to <1s since `stop()` no longer blocks on the latch timeout. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

) ## Description The Application Signals **E2E Java 25** jobs fail every validator (EMF logs, metrics, traces) because the agent produces **no telemetry at all** on Java 25. Java 11/17/21/24 are unaffected. ### Root cause The Dynamic Instrumentation feature (#1384) added `org.ow2.asm:asm:9.7` / `asm-tree:9.7` to `:awsagentprovider` for its line-level instrumentation transformer. `:awsagentprovider` relocates ASM in its **own** shadowJar, but `:otelagent` consumes it via the `javaagentLibs` configuration (the plain jar + transitive deps), so the **un-relocated** `org.objectweb.asm` 9.7 classes get bundled into the agent jar at `inst/org/objectweb/asm` — the same namespace where the upstream OpenTelemetry javaagent already ships its own (newer) ASM. The agent jar ends up with **two copies of every ASM class** (two `inst/org/objectweb/asm/ClassReader.classdata` entries: 45818 bytes for the agent's ASM, 45545 bytes for the bundled 9.7). ASM 9.7 only understands class-file versions up to Java 24 (v68). On Java 25 (v69) its `ClassReader` throws `IllegalArgumentException: Unsupported class file major version 69` for every class ByteBuddy tries to match, which **silently disables ALL instrumentation** — no spans, no App Signals metrics. Java 24 and below are within 9.7's range, which is why only Java 25 regressed. ### Fix Exclude `org.ow2.asm` from the `otelagent` `javaagentLibs` configuration. DI's plain `org.objectweb.asm.*` references then resolve to the agent's own v69-capable ASM (same `inst/org/objectweb/asm` namespace), and the duplicate is gone. This also keeps DI's ASM in lockstep with the upstream agent's ASM as new JDKs land. ## Testing Locally built the agent (`:otelagent:shadowJar`) and ran a small app (JDK `HttpServer` + an outbound `HttpURLConnection` call, which forces ByteBuddy/ASM class transformation) under the agent with the logging exporters, App Signals enabled, and DI enabled. **Jar packaging:** - Pre-fix jar: **two** `inst/org/objectweb/asm/ClassReader.classdata` entries (45818 + 45545 bytes). - Fixed jar: **one** entry (45818), no duplicate `inst/org/objectweb/asm/*` paths. **Negative control (pre-fix agent, Java 25):** 3340 × `IllegalArgumentException: Unsupported class file major version 69` from `org.objectweb.asm.ClassReader.<init>` → **0 spans, 0 HTTP metrics** emitted (app itself ran fine — telemetry silently dropped). **Fixed agent (app compiled for Java 11, run across the JDK matrix):** | JDK | SERVER+CLIENT spans | HTTP duration metrics | v69 errors | DI init | |---|---|---|---|---| | 25 | ✅ 2 | ✅ 2 | 0 | ✅ initialized | | 21 | ✅ 2 | ✅ 2 | 0 | ✅ initialized | | 17 | ✅ 2 | ✅ 2 | 0 | ✅ initialized | | 11 | ✅ 2 | ✅ 2 | 0 | ✅ initialized | Logs path also confirmed on Java 25 (`otlp.exporter.seen{type="log"}` exported; ServiceEvents OTLP logs endpoint wired). Dynamic Instrumentation logged `AWS DI: Manager initialized successfully` and shut down cleanly on every JDK. CI / Application Signals E2E across the Java matrix will provide the final cross-environment confirmation. Co-authored-by: Lei Wang <66336933+wangzlei@users.noreply.github.com>

## Summary Cherry-pick 12 commits from `main` onto `release/v2.28.x` in preparation for the v2.28.2 release. ### Included commits (in cherry-pick order) - c6f8eaa Align CompactConsoleLogRecordExporter with OTLP backend schema (#1358) - ed1150c feat(di): add Dynamic Instrumentation (opt-in, default off) (#1384) - 6bb7a97 Bump Netty to 4.1.135.Final to fix CVE-2026-45416 and CVE-2026-44249 (#1389) - 0bb3540 ci(claude-review): dedupe findings across pushes and reduce noise (#1387) - d307352 fix(di): interrupt poller threads on stop and harden flaky concurrency test (#1388) - 78364c2 feat(serviceevents): add ServiceEvents instrumentation (#1386) - 81bb002 ci(claude-review): use runtime API for author check (#1390) - 513b153 refactor(debugger): remove unused instrumentation_name field from DI (#1391) - 80b12ce serviceevents: remove adaptive sampling, default to always, cap call_path (#1392) - e48f812 serviceevents: use comma delimiter for LATENCY_THRESHOLDS (match Python/JS) (#1393) - cbb1dd7 fix(serviceevents): replace Java 9+ APIs so the agent starts on Java 8 (#1394) - 0e62f0a fix: exclude DI's bundled ASM so instrumentation works on Java 25 (#1395) The post-release version-bump commit (`e2e859b`) is intentionally excluded; the `Pre-release: Update version to 2.28.2` commit will be added separately following the standard release workflow. ## Test plan - [ ] CI passes on the release branch - [ ] Verify CHANGELOG `Unreleased` section reflects the changes shipping in v2.28.2 --------- Co-authored-by: Miqueas Herrera <vherremi@amazon.com> Co-authored-by: Prashant Srivastava <50466688+srprash@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Steve Liu <liustve@amazon.com> Co-authored-by: Mohamed Asaker <asakem@amazon.com> Co-authored-by: Vastin <3690049+vastin@users.noreply.github.com> Co-authored-by: ADOT Patch workflow <adot-patch-workflow@github.com>

srprash requested a review from a team as a code owner June 3, 2026 23:12