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 if the trace was sampled to the ctxTags #184

Merged
merged 5 commits into from
Jan 18, 2019

Conversation

domgreen
Copy link
Contributor

@domgreen domgreen commented Jan 4, 2019

Currently we add traceId and spanId to the ctxTags ... which eventually end up on each log line if you are using the ctxTags for logging ... this ends up in a very annoying situation where we have a log line with a span / trace id but when you go hunting for the trace you can never find it.

A fix to this is to add the trace.sampled to the context as well so that we can then identify that the trace will not be in our tracing system.

Previously for a sampled trace would have had :

trace.traceid:123
trace.spanid:987

Which will now be:

trace.traceid:123
trace.spanid:987
trace.sampled:true

Previously for a trace that was not samped would have had :

trace.traceid:456
trace.spanid:654

Which will now be:

trace.traceid:456
trace.spanid:654
trace.sampled:false

@domgreen domgreen added the bug label Jan 4, 2019
@domgreen domgreen self-assigned this Jan 4, 2019
@domgreen domgreen requested review from devnev, yifanzz and mwitkow January 4, 2019 15:55
@codecov-io
Copy link

codecov-io commented Jan 4, 2019

Codecov Report

Merging #184 into master will decrease coverage by 0.11%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #184      +/-   ##
==========================================
- Coverage    73.2%   73.09%   -0.12%     
==========================================
  Files          36       36              
  Lines        1340     1349       +9     
==========================================
+ Hits          981      986       +5     
- Misses        310      314       +4     
  Partials       49       49
Impacted Files Coverage Δ
tracing/opentracing/server_interceptors.go 79.06% <100%> (ø) ⬆️
tracing/opentracing/id_extract.go 90.9% <88.23%> (+6.29%) ⬆️
retry/retry.go 76.11% <0%> (-2.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4832df0...71cd4a0. Read the comment docs.

@domgreen domgreen force-pushed the only_add_trace_data_if_sampled branch from bf40426 to 438b3b1 Compare January 4, 2019 16:18
@domgreen domgreen changed the title we now will not add the traceId / spanId to ctxtags if it is not sampled Add if the trace was sampled to the ctxTags Jan 4, 2019
tracing/opentracing/id_extract.go Outdated Show resolved Hide resolved
tracing/opentracing/id_extract.go Outdated Show resolved Hide resolved
tracing/opentracing/id_extract.go Outdated Show resolved Hide resolved
@domgreen
Copy link
Contributor Author

@devnev ping

}

if strings.Contains(key, "sampled") {
t.Tags.Set(TagSampled, val)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about enforcing that the val is either true or false, and ignoring it if it isn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do that however both OpenZipkin and OpenTracing do strconv.FormatBool(sc.Sampled) to set the value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I would prefer to pass thru what is in the sampled field from the OT implementation if it is not true/false it would be valuable to have this visualised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@devnev is this change blocking a merge?

case "true", "false":
t.Tags.Set(TagSampled, val)
default:
t.Tags.Set(TagSampled, "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why set to empty value instead of leaving unset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😂 this was what you suggested during our conversation ... have updated 👍

@devnev devnev added enhancement and removed bug labels Jan 17, 2019
@domgreen domgreen merged commit f849b54 into master Jan 18, 2019
@domgreen domgreen deleted the only_add_trace_data_if_sampled branch January 18, 2019 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants