Conversation
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Adds a public OtelTracingMiddleware(OpenTelemetry) constructor so spans can be recorded on an app-owned SDK instead of GlobalOpenTelemetry, keeps the no-arg constructor's lazy global lookup, and documents the path in both EN and ZH. The change is small, additive and null-checked, and the test coverage (injected SDK isolation, nesting across scheduler hops, late global registration, noop, error status, cancel) is genuinely thorough — the approach looks right and #3229's ask is addressed. Two non-blocking points below: one documentation gap around the JVM-wide Reactor hook the new constructor still registers, one tracer-caching nit, plus a CI/checklist note. Not approving yet: the checklist's format/test boxes are explicitly "Not run" and the build matrix was still pending at review time.
Automated review by github-manager-bot
| public OtelTracingMiddleware(OpenTelemetry openTelemetry) { | ||
| this.openTelemetry = | ||
| Objects.requireNonNull(openTelemetry, "openTelemetry must not be null"); | ||
| registerReactorHook(); |
There was a problem hiding this comment.
[Warning] This new constructor also runs registerReactorHook(), i.e. ContextPropagationOperator...registerOnEachOperator() — a JVM-wide Reactor hook that wraps every operator of every Flux/Mono in the process, independent of which SDK is used for tracer lookup. So "app-owned SDK" only isolates tracer lookup, not the global instrumentation side effect; an app that adopts this constructor precisely to avoid touching global state still gets the hook. Suggest either documenting the side effect in this Javadoc (plus the middleware docs), or adding an overload/flag that lets the caller decide whether the Reactor hook should be registered.
|
|
||
| private Tracer getTracer() { | ||
| if (openTelemetry != null) { | ||
| return openTelemetry.getTracer(INSTRUMENTATION_NAME); |
There was a problem hiding this comment.
[Info] getTracer() is called once per span creation. For the app-owned instance this hits the provider every time: SdkTracerProvider.get() is a concurrent-map lookup, but the SDK still allocates an obfuscated tracer wrapper on each call. Caching the Tracer per OpenTelemetry instance would remove that repeated work on the onModelCall / onActing hot paths, while keeping the deliberate lazy lookup for the global (no-arg) path.
| * runs. When only the default no-op provider is active, the hooks still run the downstream | ||
| * chain. | ||
| */ | ||
| public OtelTracingMiddleware() { |
There was a problem hiding this comment.
[Info] The PR checklist has mvn spotless:apply and mvn test marked "Not run". validate is green and the ubuntu/windows build matrix jobs were still queued when this review was posted, so formatting/tests will be settled by CI — but please tick those boxes (or push a reformat commit) before merge so the record matches.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Thanks for the review. Both notes are addressed in c582368.
|
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
The new commit documents the JVM-wide Reactor hook and caches the app-owned tracer. The cache is safely published, the no-argument path keeps lazy global lookup, and the hook side effect is now disclosed in both language docs — previous-round findings on hook disclosure and tracer lookup are resolved. One wording nit below; nothing blocking.
Previous-round follow-up
Earlier inline findings are addressed; the remaining CI-status item is a PR-process matter, not a source change.
Automated review by github-manager-bot
|
|
||
| Passing `appSdk` does not replace `GlobalOpenTelemetry` and does not change `StudioManager`. `StudioManager` still installs deprecated `TracerRegistry` tracing on its own during `initialize()`. Vendor instrumentation that rewrites tracer lookup, and a missing Studio call-tree expansion control, need separate verification against the deployment that produces them. This middleware selects which OpenTelemetry SDK records the `onAgent`, `onModelCall`, and `onActing` spans. `OpenTelemetry.noop()` is also a valid argument: the downstream chain still runs, and no spans are recorded. | ||
|
|
||
| Constructing `OtelTracingMiddleware` — either `new OtelTracingMiddleware()` or `new OtelTracingMiddleware(appSdk)` — also registers a JVM-wide Reactor hook, `ContextPropagationOperator.registerOnEachOperator()`, the first time any instance is created. The hook wraps every operator of every `Flux` and `Mono` in the process so parent spans survive `publishOn` / `subscribeOn` hops. It is independent of which SDK records spans: an application-owned SDK isolates tracer lookup from `GlobalOpenTelemetry`, not this instrumentation side effect. The hook is installed at most once per JVM and is not removed when the middleware or the SDK is closed. |
There was a problem hiding this comment.
Reactor's onEachOperator hook decorates operators assembled after it is registered; it cannot retroactively wrap Flux/Mono chains that were already assembled. Qualify the claim that it wraps every operator in the process (and make the same correction in the Chinese documentation) so applications do not assume pre-assembled publishers gain propagation.
This branch has not been deployed
2 participants
AgentScope-Java Version
Add a public OtelTracingMiddleware(OpenTelemetry openTelemetry) constructor with a non-null contract and caller-owned lifecycle; use its io.agentscope tracer for all three existing hooks through getTracer(), without registering or replacing the global SDK. Preserve the no-argument constructor's lazy global lookup at hook execution, including middleware constructed before global registration, and preserve the existing once-per-JVM Reactor hook registration for both constructors. Keep the existing parent-context propagation, attributes, cancellation, and error behavior unchanged; no extra provider interface, builder, or test-only seam is needed.
The reporter uses StudioManager with a self-hosted agent-studio and reports missing traces when AliyunJavaAgent replaces the return value of TracerRegistry.get(). Disabling the agent's AgentScope instrumentation restores exported traces but reportedly loses the UI's expandable call tree; the UI cause has not been verified. The latest reporter comment explicitly requests a supported OpenTelemetry/Tracer injection option on OtelTracingMiddleware, in addition to describing the broader StudioManager coexistence goal. The local middleware exposes only a no-argument constructor and obtains every tracer from GlobalOpenTelemetry, preventing selection of an independent application SDK.
Fixes #3229
Description
Add a public OtelTracingMiddleware(OpenTelemetry openTelemetry) constructor with a non-null contract and caller-owned lifecycle; use its io.agentscope tracer for all three existing hooks through getTracer(), without registering or replacing the global SDK. Preserve the no-argument constructor's lazy global lookup at hook execution, including middleware constructed before global registration, and preserve the existing once-per-JVM Reactor hook registration for both constructors. Keep the existing parent-context propagation, attributes, cancellation, and error behavior unchanged; no extra provider interface, builder, or test-only seam is needed.
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn -pl agentscope-core spotless:checkpasses on c582368.mvn test)Full
mvn testnot run locally.OtelTracingMiddlewareTestpasses 19/19 on c582368.