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 TraceState to SpanContext in API #1340

Merged
merged 15 commits into from
Dec 21, 2020

Conversation

matej-g
Copy link
Contributor

@matej-g matej-g commented Nov 15, 2020

The specification on SpanContextrequires that TraceState is part of the of the tracing API.

This PR adds TraceState to SpanContext, implements operations according to the spec and adjusts some related parts of the code base (OTLP span transform, SDK test, stdout exporter test).

Some parts based on the previously umerged PR #81.

Does not include changes to propagators, however, if required they can be done as part of this PR as well.

Resolves #1302.

@codecov
Copy link

codecov bot commented Nov 15, 2020

Codecov Report

Merging #1340 (24d9b4a) into master (3521526) will increase coverage by 0.1%.
The diff coverage is 94.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1340     +/-   ##
========================================
+ Coverage    78.3%   78.5%   +0.1%     
========================================
  Files         125     125             
  Lines        6259    6324     +65     
========================================
+ Hits         4906    4966     +60     
- Misses       1110    1114      +4     
- Partials      243     244      +1     
Impacted Files Coverage Δ
oteltest/tracer.go 93.0% <81.2%> (-7.0%) ⬇️
trace/trace.go 86.6% <96.0%> (+6.6%) ⬆️
exporters/otlp/internal/transform/span.go 98.0% <100.0%> (+<0.1%) ⬆️
oteltest/span.go 100.0% <100.0%> (ø)
sdk/trace/span.go 91.4% <100.0%> (+0.1%) ⬆️

@matej-g matej-g marked this pull request as ready for review November 15, 2020 17:45
trace/trace.go Outdated Show resolved Hide resolved
trace/trace.go Outdated Show resolved Hide resolved
trace/trace.go Outdated Show resolved Hide resolved
- Adjust tests for trace API
- Adjust adjacent parts of codebase (test utils, SDK etc.)
trace/trace.go Show resolved Hide resolved
trace/trace.go Show resolved Hide resolved
trace/trace.go Show resolved Hide resolved
trace/trace.go Outdated Show resolved Hide resolved
trace/trace_test.go Outdated Show resolved Hide resolved
trace/trace_test.go Outdated Show resolved Hide resolved
trace/trace_test.go Show resolved Hide resolved
@MrAlias
Copy link
Contributor

MrAlias commented Dec 2, 2020

I'm looking at this reference implementation of the tracestate and I'm wondering if we want to take a similar approach the TraceState type. Having it itself be a slice would allow the user of this to interact with the data structure in idiomatic Go ways instead of requiring us to build out all the getters/setters.

I think the only outstanding issue would be around how to handle invalid (e.g. too many members, invalid members) tracestate when serializing back into the header.

@matej-g
Copy link
Contributor Author

matej-g commented Dec 3, 2020

Having it itself be a slice would allow the user of this to interact with the data structure in idiomatic Go ways instead of requiring us to build out all the getters/setters.

Unless I'm missing something, we would not want the user to directly interact with the slice, as the API spec puts some limitations on TraceState manipulation (e.g. is immutable, must be valid at all times, must validate input params etc.), which we can control with get / add / delete methods.

@MrAlias
Copy link
Contributor

MrAlias commented Dec 3, 2020

Unless I'm missing something, we would not want the user to directly interact with the slice, as the API spec puts some limitations on TraceState manipulation (e.g. is immutable, must be valid at all times, must validate input params etc.), which we can control with get / add / delete methods.

Ah, makes sense. Thanks for the link to the specification, TIL. 👍

- for type SpanContext, if SpanID, TraceID, TraceFlag and TraceState are
equal
- for type TraceState, if entries of both respective trace states are
equal

Signed-off-by: Matej Gera <matejgera@gmail.com>
@matej-g
Copy link
Contributor Author

matej-g commented Dec 8, 2020

@MrAlias thanks for the feedback ❤️, I updated parts which are clear, waiting for clarifications on the rest.

trace/trace_test.go Outdated Show resolved Hide resolved
oteltest/tracer.go Outdated Show resolved Hide resolved
trace/trace.go Show resolved Hide resolved
trace/trace.go Outdated Show resolved Hide resolved
trace/trace.go Show resolved Hide resolved
Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

None of my feedback should be considered blocking. If you agree with the suggestion to use label.Set and its map-key equality support, that could be a TODO. The suggestion to move IsEqualWith() into a test package feels important to do now. I'll approve and let the maintainers decide. :-)

- Move IsEqualWith method to be only in test package
- Minor improvements, typos etc.
@MrAlias MrAlias merged commit 439cd31 into open-telemetry:master Dec 21, 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.

API: Add TraceState to SpanContext
5 participants