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.

Sign up for GitHub

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

Jump to bottom

Completely revamp tracing spans #1205

Merged
merged 45 commits into from
Sep 4, 2019
Merged

Completely revamp tracing spans #1205

merged 45 commits into from
Sep 4, 2019

Conversation

dansanduleac
Copy link
Contributor

@dansanduleac dansanduleac commented Sep 3, 2019 •
edited by iamdanfox
Loading

Before this PR

Spans for a single request's lifetime are difficult to correlate as they use different trace ids per attempt, and it's not obvious when additional attempts are being made.

After this PR

==COMMIT_MSG==
For a single JaxRsClient request, all spans are now correctly attributed to a single traceId.
==COMMIT_MSG==

both jax-rs and retrofit now trace things correctly, including ListenableFuture/CompletableFuture calls:

image

This PR uses release candidates tagged on this branch: palantir/tracing-java#124

Possible downsides?

  • more complexity in c-j-r
  • we're touching some hot codepaths here, so any unintended performance regressions could have quite an impact.

@changelog-app
Copy link

changelog-app bot commented Sep 3, 2019 •
edited by iamdanfox
Loading

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

For a single JaxRsClient request, all spans are now correctly attributed to a single traceId. Newly introduced spans are:

  • OkHttp: Attempt X (first one will be Attempt 0, max is still 4 retries for a total of 5 reqs)
  • OkHttp: client-side-concurrency-limiter 4/10 (showing number of inflight request / current limit)
  • OkHttp: dispatcher (signifies a client may have hit the max requests per host)
  • OkHttp: backoff-with-jitter
  • OkHttp: wait-for-headers
  • OkHttp: wait-for-body

Check the box to generate changelog(s)

  • Generate changelog entry

@dansanduleac
Copy link
Contributor Author

Looking good:
image

The long times at the beginning of each attempt are the backoff times for each retry.

dansanduleac and others added 19 commits September 3, 2019 19:39
@dansanduleac
can't be null
05da94b
@dansanduleac
path template as name of parent span
8561dc5
@dansanduleac
tracing rc3 to get getters on CloseableSpan
c722b04
@dansanduleac
copy in OkhttpTraceInterceptor and clean up internal span name
142e6ee
@dansanduleac
move backoff span creation into scheduleExecution, fix throttle case
656b0d1
@dansanduleac
abstract out creating next attempt
3a8a171
@dansanduleac
checkstyle
1e3a95d
@dansanduleac
no need for execute() anymore
d7e485b
@dansanduleac
bring in junit5
6001ea4
@dansanduleac
ensure guava 28 across compileOnly/annotationProcessor too, otherwise…
4bcafd6 
… intellij breaks
@dansanduleac
Attempt to convert TracerTest to trace-snapshotting junit5 tests
b302343
@dansanduleac
Revert "Attempt to convert TracerTest to trace-snapshotting junit5 te…
4a84280 
…sts"

This reverts commit b302343
@dansanduleac
Revert "ensure guava 28 across compileOnly/annotationProcessor too, o…
cc8b2aa 
…therwise intellij breaks"

This reverts commit 4bcafd6
@dansanduleac
Revert "bring in junit5"
5d0ea3a 
This reverts commit 6001ea4
@dansanduleac
OkHttp: network-call
7ea8323
@dansanduleac
try to fix test, fail?
a8a622e
@dansanduleac
backoff-with-jitter
9d7b2de
@iamdanfox
Bump to rc4
fbce0dc
@iamdanfox
Use new OkhttpTraceInterceptor2
3053407
@iamdanfox iamdanfox changed the title Use new detached spans Completely revamp tracing spans Sep 4, 2019
@iamdanfox iamdanfox marked this pull request as ready for review September 4, 2019 18:35
@iamdanfox iamdanfox requested a review from a team as a code owner September 4, 2019 18:35
iamdanfox
iamdanfox reviewed Sep 4, 2019
View reviewed changes
ferozco
ferozco reviewed Sep 4, 2019
View reviewed changes
okhttp-clients/src/main/java/com/palantir/conjure/java/okhttp/SettableDispatcherSpan.java Outdated

@Value.Modifiable
@ImmutablesStyle
interface SettableDispatcherSpan {
Copy link
Contributor

Choose a reason for hiding this comment

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

These could probably use some docs explaining how they're used

@ferozco
Copy link
Contributor

ferozco commented Sep 4, 2019

👍

@bulldozer-bot bulldozer-bot bot merged commit 98524f5 into develop Sep 4, 2019
@bulldozer-bot bulldozer-bot bot deleted the ds/use-new-tracing branch September 4, 2019 19:29
@svc-autorelease
Copy link
Collaborator

Released 4.36.0

@zachkirsch zachkirsch mentioned this pull request Sep 22, 2019
Merged
@iamdanfox iamdanfox mentioned this pull request Aug 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iamdanfox iamdanfox iamdanfox left review comments

@ferozco ferozco ferozco left review comments

Assignees
No one assigned
Labels
autorelease merge when ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dansanduleac @ferozco @svc-autorelease @iamdanfox