Navigation Menu

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

Add instrumentation to propagate across scala promises properly #1824

Closed
wants to merge 3 commits into from

Conversation

tylerbenson
Copy link
Contributor

There were significant changes in 2.13, so that required completely separate instrumentation.

@tylerbenson
Copy link
Contributor Author

Looks like this broke the play tests...

@richardstartin
Copy link
Member

Could we add to the play smoke test and verify the parent/child structure of the traces?

@bantonsson
Copy link
Contributor

I'm having a hard time reviewing these types of things, since I can't figure out how we want an async context to propagate. In some places we add the scope at creation time like this instrumentation, and in some places we add it at scheduling time with the isAsyncPropagating flag, but that won't overwrite the scope we captured at creation time (if any). The interactions and intent is very unclear to me.

So the play test failure is likely related to the fact that the CallbackRunnable will be executed on the specialized InternalCallbackExecutor that we try to instrument, but it executes other Runnable in batch, so the continuations that we now grab at CallbackRunnable creation time might not be closed properly.

There were significant changes in 2.13, so that required completely separate instrumentation.
@tylerbenson
Copy link
Contributor Author

@bantonsson the general idea is that a promise should propagate the current span when the callback is created to the scope when the callback executes. This is different from the previous behavior where the callback would be in the scope of whatever completes the promise. (Creation vs Completion)

@tylerbenson tylerbenson marked this pull request as ready for review September 8, 2020 20:07
@tylerbenson tylerbenson requested a review from a team as a code owner September 8, 2020 20:07
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.

@tylerbenson I don't think that this should go in right now, without the change in behavior being fully understood by more application tests, and being mirrored for CompletableFuture in java, since a mixed environment like play for java which is running scala under the hood, would have a mixed behavior between the framework defaults (scala) and the user code (java). I added a test for CompletableFuture here #1846

As for the change in behavior, previously the scope was at the point of being added to an executor which might not be the scope completing the promise depending on how we propagate the scope across async boundaries. This seems to be the issue that I'm currently facing in my failing play-2.5 smoke tests, where we drop the scope somewhere before the actual play action is created and executed.

I've tried these changes on top of my currently failing play-2.5 smoke test and they make no difference for them, very likely because it's a play for java application, but also because of the async gap that I'm trying to find.

@dougqh
Copy link
Contributor

dougqh commented Sep 9, 2020

@tylerbenson I don't think that this should go in right now, without the change in behavior being fully understood by more application tests, and being mirrored for CompletableFuture in java, since a mixed environment like play for java which is running scala under the hood, would have a mixed behavior between the framework defaults (scala) and the user code (java). I added a test for CompletableFuture here #1846

I haven't analyzed this situation enough to have a well-formed opinion, but I do think in general we need to be more careful than we have been. I think we're playing a bit of whack-a-mole on async issues, and overall, that's not translating to significant progress.

I think we really need to verify everything from the bottom up, so that we make sure we're fixing the root problem and not just the symptom.

@tylerbenson
Copy link
Contributor Author

we're fixing the root problem and not just the symptom.

This is what I was trying to do... I feel promises/futures are at the root of a significant propagation problem that we can't solve otherwise.

@bantonsson
Copy link
Contributor

@tylerbenson I completely agree that promise instrumentation is an area that we need to address. I'll add more tests and instrumentation to #1846, or if you wan't we could merge it into this branch.

@bantonsson
Copy link
Contributor

Both Scala and Java promises (CompletableFuture) will be done together in #1846

@bantonsson bantonsson closed this Sep 10, 2020
@bantonsson bantonsson deleted the tyler/scala-promise branch September 10, 2020 14:46
@tylerbenson tylerbenson added this to the Closed milestone Sep 11, 2020
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.

None yet

4 participants