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

Refactor Tracestate #1931

Merged
merged 13 commits into from
May 24, 2021
Merged

Refactor Tracestate #1931

merged 13 commits into from
May 24, 2021

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented May 19, 2021

The main purpose of these changes is to restructure the TraceState type around a list of strings. The methods have all been updated to accept key/value values as string instead of attribute.KeyValue.

  • Move TraceStateFromKeyValues from the trace API to the oteltest package.
  • Update TraceState.MarshalJSON to return the tracestate string.
  • Move tracestate header parsing logic to the tracestate.go file in the trace package.
  • Move the TraceState type and associated code to its own file.
  • Correct the tracestate list regex to allow leading and trailing whitespace in accordance with the W3C Trace Context specification.
  • Add a ParseTraceState function that attempts to decode a TraceState from a passed string
  • Change the TraceState.Delete method to not return an error. The only time it would return an error is when the user passed an invalid key. Now it "handles" this error by not deleting the member associated with the invalid key (something that can never be in a TraceState) and just returns a copy of the TraceState.
  • Remove the TraceState.IsEmpty method in favor of the added TraceState.Len method. This new method has a more diverse use and solves the same use case the original did.

Part of #1539

@MrAlias MrAlias added area:trace Part of OpenTelemetry tracing release:1.0.0-rc.1 labels May 19, 2021
@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #1931 (b4b2f6c) into main (d3b1280) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #1931   +/-   ##
=====================================
  Coverage   79.5%   79.5%           
=====================================
  Files        139     141    +2     
  Lines       7463    7464    +1     
=====================================
+ Hits        5937    5941    +4     
+ Misses      1278    1275    -3     
  Partials     248     248           
Impacted Files Coverage Δ
trace/trace.go 83.6% <ø> (-3.2%) ⬇️
oteltest/tracestate.go 100.0% <100.0%> (ø)
propagation/trace_context.go 64.9% <100.0%> (-4.8%) ⬇️
trace/tracestate.go 100.0% <100.0%> (ø)

@MrAlias MrAlias added this to the RC1 milestone May 20, 2021
@MrAlias
Copy link
Contributor Author

MrAlias commented May 20, 2021

One thing I considered when making these changes, and am still not sure if it would make this better, is to move the trace state stuff into it's own package. I would rename the TraceState type to List to match the W3C specification and change ParseTraceState to just Parse. That would mean calling tracestate.Parse would return a tracestate.List. We could even export the Member type and have it be structured in a way, similar to this but made static, that it would contain vendor and tenant information instead of just a Key.

The issues I was not able to resolve for this approach were:

  • Where should the tracestate package live?
  • What about the rest of the Trace Context specification? This would seem to be an incomplete implementation of the specification.

Copy link
Contributor

@MadVikingGod MadVikingGod left a comment

Choose a reason for hiding this comment

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

Overall looks good.

I don't see a good reason to move it right now, but if I were to move it I would consider either trace/tracestate, or metadata/tracestate. The former because it is solely a concern of traces, the latter because it could be a very convenient place to put all of the metadata's we are working with: tracestate, baggage, and attributes come to mind.

propagation/trace_context.go Show resolved Hide resolved
trace/tracestate_test.go Outdated Show resolved Hide resolved
Remove circularity of TestTraceStateLen.
@MrAlias MrAlias merged commit 0eeb8f8 into open-telemetry:main May 24, 2021
@MrAlias MrAlias deleted the tracestate-string branch May 24, 2021 14:53
@Aneurysm9 Aneurysm9 mentioned this pull request Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:trace Part of OpenTelemetry tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants