-
Notifications
You must be signed in to change notification settings - Fork 867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
End span on cancellation of subscription to reactive publishers #3153
Merged
trask
merged 12 commits into
open-telemetry:main
from
HaloFour:withspan-reactive-cancellation
Jun 3, 2021
Merged
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
acf1edd
Stop span on cancellation of subscription to reactive publisher
HaloFour 78d8378
Fix test name
HaloFour 25b2a20
Add semantic attribute on cancelation of reactive publisher
HaloFour b5a332a
Fix span attribute name
HaloFour 8bbba3b
Change TracingOperator and TracingAssembly to accept configuration fr…
HaloFour 7a94c9b
Remove unused import
HaloFour 2db515a
Remove private constructor
HaloFour 8790578
Fix test
HaloFour 712bf7f
Fix Lettuce tests
HaloFour d3cfc4b
Add javaagent tests
HaloFour 3a2350b
fix copypasta fail
HaloFour b4e7a6b
Refactor RxJava 2/3 javaagent classes to javaagent package
HaloFour File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
22 changes: 22 additions & 0 deletions
22
...ain/java/io/opentelemetry/instrumentation/reactor/ReactorAsyncSpanEndStrategyBuilder.java
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.reactor; | ||
|
||
public final class ReactorAsyncSpanEndStrategyBuilder { | ||
private boolean captureExperimentalSpanAttributes; | ||
|
||
ReactorAsyncSpanEndStrategyBuilder() {} | ||
|
||
public ReactorAsyncSpanEndStrategyBuilder setCaptureExperimentalSpanAttributes( | ||
boolean captureExperimentalSpanAttributes) { | ||
this.captureExperimentalSpanAttributes = captureExperimentalSpanAttributes; | ||
return this; | ||
} | ||
|
||
public ReactorAsyncSpanEndStrategy build() { | ||
return new ReactorAsyncSpanEndStrategy(captureExperimentalSpanAttributes); | ||
} | ||
} |
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
22 changes: 22 additions & 0 deletions
22
...ibrary/src/main/java/io/opentelemetry/instrumentation/reactor/TracingOperatorBuilder.java
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.reactor; | ||
|
||
public final class TracingOperatorBuilder { | ||
private boolean captureExperimentalSpanAttributes; | ||
|
||
TracingOperatorBuilder() {} | ||
|
||
public TracingOperatorBuilder setCaptureExperimentalSpanAttributes( | ||
boolean captureExperimentalSpanAttributes) { | ||
this.captureExperimentalSpanAttributes = captureExperimentalSpanAttributes; | ||
return this; | ||
} | ||
|
||
public TracingOperator build() { | ||
return new TracingOperator(captureExperimentalSpanAttributes); | ||
} | ||
} |
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, do we need the builder for this class? Is it supposed to be possible to use it without
TracingOperator
?If we can we can treat it as an internal part of the library instrumentation then we can make it package-private and remove the builder and use a simple
ReactorAsyncSpanEndStrategy(boolean)
constructor instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I had not considered them as being internal to the instrumentation. Would it be problematic to allow them to be used directly? They were already public even though they were singletons.
As of right now I am using the ReactorAsyncSpanEndStrategy without the TracingOperator in my Spring WebFlux projects. I've been slowly migrating away from our own instrumentation but I've not yet considered replacing our own Reactor operators, partially because they do more than propagate the OTel Context. I've combined several operations into a single operator because every individual operator effectively doubles the stack trace as they are added for every reactive operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it would be a bit better to expose less API surface, and they seemed like they could be internal. I wouldn't say that it's a hard requirement for them to be internal; I feel like they should be, but not have to. If you're using them right now then it's probably fine to keep them public for a while.