Skip to content

Enable async tracking for some non-async frameworks#316

Merged
tylerbenson merged 1 commit into
masterfrom
tyler/async-flag
May 10, 2018
Merged

Enable async tracking for some non-async frameworks#316
tylerbenson merged 1 commit into
masterfrom
tyler/async-flag

Conversation

@tylerbenson
Copy link
Copy Markdown
Contributor

This will allow tracing of additional work being done inside the servlet context.

Closes #304

This will allow tracing of additional work being done inside the servlet context.
@tylerbenson tylerbenson added type: enhancement Enhancements and improvements inst: others All other instrumentations labels May 9, 2018
@tylerbenson tylerbenson added this to the 0.8.0 milestone May 9, 2018
@tylerbenson tylerbenson requested a review from realark May 9, 2018 00:42
@realark
Copy link
Copy Markdown
Contributor

realark commented May 9, 2018

I think we'll also want to turn off the async propagation when the root span finishes.

if (scope instanceof TraceScope) {
  ((TraceScope) scope).setAsyncPropagation(false);
}

@tylerbenson
Copy link
Copy Markdown
Contributor Author

tylerbenson commented May 10, 2018

@realark I didn't worry about that because the async propagation flag is on the scope, which is recreated every request and goes away when that scope is closed. If we add the flag to turn it off, it kinda clutters the code.

Copy link
Copy Markdown
Contributor

@realark realark left a comment

Choose a reason for hiding this comment

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

👍 Good point, once the scope is closed the async propagation will finish so it's not required to turn it off.

@tylerbenson tylerbenson merged commit 632d9b8 into master May 10, 2018
@tylerbenson tylerbenson deleted the tyler/async-flag branch May 10, 2018 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inst: others All other instrumentations type: enhancement Enhancements and improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants