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

Adds key traceSampled to MDC when set trace is observable #104

Closed
wants to merge 2 commits into from

Conversation

dsd987
Copy link
Contributor

@dsd987 dsd987 commented Mar 26, 2019

Before this PR

Log consumers were unable to tell which traces were sampled (observable: true) vs which traces were not sampled (observable: false).

After this PR

An MDC key traceSampled will now appear with the value "1" when the set trace has been sampled (observable: true). If the set trace is not sampled (observable: false), the key will not appear in the MDC.

…t to 1 if observed and not present when not observed.
@dsd987 dsd987 requested a review from a team as a code owner March 26, 2019 14:04
@dsd987
Copy link
Contributor Author

dsd987 commented Mar 26, 2019

Proposed for #103

Copy link
Contributor

@carterkozak carterkozak left a comment

Choose a reason for hiding this comment

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

Looks good, commented a few nits.

tracing/src/main/java/com/palantir/tracing/Tracer.java Outdated Show resolved Hide resolved
tracing/src/main/java/com/palantir/tracing/Tracer.java Outdated Show resolved Hide resolved

private static void setTraceSampledMdcIfObservable(boolean isObservable) {
if (isObservable) {
MDC.put(Tracers.TRACE_SAMPLED_KEY, "1");
Copy link
Contributor

Choose a reason for hiding this comment

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

Might add a comment describing why we're using 1 instead of true.

@@ -26,6 +26,7 @@
public final class Tracers {
/** The key under which trace ids are inserted into SLF4J {@link org.slf4j.MDC MDCs}. */
public static final String TRACE_ID_KEY = "traceId";
public static final String TRACE_SAMPLED_KEY = "traceSampled";
Copy link
Contributor

Choose a reason for hiding this comment

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

We should document this key similarly to TRACE_ID_KEY and describe possible values.

@carterkozak
Copy link
Contributor

@uschi2000 I'm curious of your thoughts on this.

@dsd987
Copy link
Contributor Author

dsd987 commented Mar 26, 2019

Any thoughts on having the value of "0" when the trace is not sampled vs just not having the key? Would be storing more but would be more consistent with the use of the http header.

@carterkozak
Copy link
Contributor

On hold pending #103 (comment)

bulldozer-bot bot pushed a commit that referenced this pull request Apr 17, 2019
…le (#144)

## Before this PR
Log consumers were unable to tell which traces were sampled (observable: true) vs which traces were not sampled (observable: false).

## After this PR
An MDC key traceSampled will now appear with the value "1" when the set trace has been sampled (observable: true). If the set trace is not sampled (observable: false), the key will not appear in the MDC.

Note: this replaces #104
@carterkozak
Copy link
Contributor

Closing in favor of #144

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

2 participants