Skip to content
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

rxJava 2 instrumentation #2053

Merged
merged 6 commits into from Nov 16, 2020
Merged

rxJava 2 instrumentation #2053

merged 6 commits into from Nov 16, 2020

Conversation

mcculls
Copy link
Contributor

@mcculls mcculls commented Nov 5, 2020

This instrumentation takes roughly the same approach as the latest reactor instrumentation:

  • active parent spans are captured when observables are created
  • captured parent spans are activated when the observable sends events to observers
  • no spans are created for the observables themselves

The general idea is to propagate context so that spans resulting from reactive work created under an active span use that span as their parent, even if the reactive work is triggered later on in the application.

In the future it would be nice to store some kind of continuation structure in the context store rather than the parent span. I did look into using State/ConcurrentState but ran into activation issues when an observable sends multiple events or has multiple observers - so I'd prefer to get this initial approach merged first (with tests) and then iterate on it.

@william-tran
Copy link
Contributor

Speak of the devil, I was just looking at issues around not seeing traces propagate in Java Specialagent after using an RxJava2 based MQTT client. Your implementation is quite different from https://github.com/opentracing-contrib/java-specialagent/tree/master/rule/rxjava-2. Is that because Specialagent is missing a lot of cases?

@mcculls
Copy link
Contributor Author

mcculls commented Nov 6, 2020

@william-tran I haven't looked at that instrumentation, but afaict it only handles observables and not all of the other rxJava constructions. Also the goal of this particular instrumentation is not so much to do with adding named spans for each step in the stream, but propagating the relevant context around.

@mcculls mcculls force-pushed the mcculls/rxJava2 branch 2 times, most recently from d9cfaa6 to 099a571 Compare November 12, 2020 20:05
@mcculls mcculls changed the title rxJava 2 instrumentation (WIP) rxJava 2 instrumentation Nov 13, 2020
@mcculls mcculls marked this pull request as ready for review November 13, 2020 14:32
@mcculls mcculls requested a review from a team as a code owner November 13, 2020 14:32
Copy link
Contributor

@bantonsson bantonsson left a comment

Choose a reason for hiding this comment

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

I get a slightly uneasy feeling from Spans being moved around and reactivated, and potentially finished early by other code. Looking forward to the follow-up with State.

@richardstartin
Copy link
Member

@bantonsson For the record, that's my position too.

@mcculls
Copy link
Contributor Author

mcculls commented Nov 13, 2020

FWIW I agree the eventual solution will involve something that looks very much like State but fits better with the reactive model - at which point we can also apply it to the reactor instrumentation.

@mcculls mcculls merged commit 4e4023e into master Nov 16, 2020
@mcculls mcculls deleted the mcculls/rxJava2 branch November 16, 2020 09:19
@github-actions github-actions bot added this to the 0.68.0 milestone Nov 16, 2020
@richardstartin richardstartin added the inst: others All other instrumentations label Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inst: others All other instrumentations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants