Skip to content

Re-register Reactor hook after Hooks.resetOnEachOperator() - #20845

Merged
trask merged 3 commits into
open-telemetry:mainfrom
DDINGJOO:reactor-sinks-reset-all-hooks
Oct 24, 2026
Merged

trask merged 3 commits into
open-telemetry:mainfrom
DDINGJOO:reactor-sinks-reset-all-hooks

Conversation

@DDINGJOO

Copy link
Copy Markdown
Contributor

After an application calls Hooks.resetOnEachOperator(), the agent no longer propagates the context through Reactor operators: a subscriber of an externally driven Flux.create publisher no longer sees the context that was current when it subscribed, and nothing brings it back.

HooksInstrumentation is meant to prevent that: it re-registers the hook at the end of Hooks.resetOnEachOperator. But ContextPropagationOperator.registerOnEachOperator() returns early when its static enabled flag is set, and the application's own reset doesn't go through the operator, so the flag stays set and the registration is ignored.

This adds a second advice for the no-argument resetOnEachOperator() that resets the operator state and registers again. The keyed resetOnEachOperator(String) and the type initializers keep the existing advice. There is no API change.

HooksResetTest calls Hooks.resetOnEachOperator() twice and checks the context after each; it fails without the change (the second assertion, after the first reset), and also checks that resetting an unrelated hook key changes nothing.

The agent installs its onEachOperator hook again when the application resets
the hooks, but ContextPropagationOperator still considered the hook
registered and ignored the registration, so the context stopped being
propagated after Hooks.resetOnEachOperator().

Assisted-by: Claude Opus 5.5
@DDINGJOO
DDINGJOO requested a review from a team as a code owner October 22, 2026 08:24
@opentelemetry-pr-dashboard
opentelemetry-pr-dashboard Bot requested a balanced review from Copilot October 22, 2026 08:24
@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Pull request dashboard status

Merged · refreshed 2026-10-24 23:06 UTC

Status above doesn't look right?
  • Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Re-registering a newly created operator can accumulate live async-strategy registrations when publishers survive repeated resets.

1 open issue
What changed in this PR

Restores Reactor context propagation after applications call Hooks.resetOnEachOperator(), without changing the public API.

Changes:

  • Adds dedicated advice to reset operator state and re-register the hook after a no-argument reset.
  • Adds regression tests for repeated full resets and an unrelated keyed reset.
File Description
instrumentation/​reactor/​reactor-3.1/​javaagent/​src/​test/​java/​io/​opentelemetry/​javaagent/​instrumentation/​reactor/​v3_1/​HooksResetTest.java Tests context propagation after hook resets.
instrumentation/​reactor/​reactor-3.1/​javaagent/​src/​main/​java/​io/​opentelemetry/​javaagent/​instrumentation/​reactor/​v3_1/​HooksInstrumentation.java Separates full-reset advice from initialization and keyed-reset advice.

Review effort: Balanced

Re-registering with a newly built operator after Hooks.resetOnEachOperator()
added a new async operation end strategy to the registry on every reset,
since each operator owns its own strategy and the registry unregisters
by identity. The agent now keeps one operator and resets and registers
that same operator.

Assisted-by: Claude Opus 5.5

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The reset lifecycle and focused regression coverage address the defect, with only non-blocking convention suggestions remaining.

1 open issue
1 previously missed issue

In code that hasn't changed since last review

Low severity Match resetOnEachOperator(String) with takesArguments(String.class)

instrumentation/​reactor/​reactor-3.1/​javaagent/​src/​main/​java/​io/​opentelemetry/​javaagent/​instrumentation/​reactor/​v3_1/​HooksInstrumentation.java:32

[Javaagent] Non-blocking: the keyed overload has the fixed signature resetOnEachOperator(String). The matching rule in .github/instructions/javaagent.instructions.md requires takesArguments(Type...) for fixed signatures using accessible java.base types. Use takesArguments(String.class) instead of matching only the argument count.

1 resolved since last review

Review effort: Balanced

The field holds a registration helper rather than an immutable value,
so it follows the lower camel case rule for fields.

Assisted-by: Claude Opus 5.5

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused fix and regression coverage address the reset lifecycle, with only a non-blocking matcher-convention suggestion remaining.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Low severity Use typed matcher for keyed overload

instrumentation/​reactor/​reactor-3.1/​javaagent/​src/​main/​java/​io/​opentelemetry/​javaagent/​instrumentation/​reactor/​v3_1/​HooksInstrumentation.java:32

[Javaagent] Non-blocking: match the keyed overload with takesArguments(String.class) rather than its argument count. The fixed-signature rule in .github/instructions/javaagent.instructions.md:71–73 requires typed matchers for accessible java.base parameters, even when the advice does not use them.

1 resolved since last review

Review effort: Balanced

@laurit laurit added this to the v3.0.0 milestone Oct 22, 2026
@trask
trask merged commit 0f4b842 into open-telemetry:main Oct 24, 2026
69 checks passed
@otelbot

otelbot Bot commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

Thank you for your contribution @DDINGJOO! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants