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

Decouple the trace id struct generation from its encoding #3019

Merged
merged 1 commit into from
Apr 24, 2023

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented Apr 13, 2023

This PR was triggered by this comment from @mstoykov. It intends to refactor the trace id structure, and its encoding method in a way that decouples them from each other.

Trace ids are serialised as 16 byte slices, which are in turn encoded into hexadecimal strings that are attached to requests HTTP headers. Some parts of the trace id have a fixed size, some others, a varying one (time, and random suffix). Because some of its parts have a varying size, we need to start serializing it to find out how much space is the random suffix gonna take.

Thus, in order to decouple the trace id creation from its encoding, we have made the trace id struct private (traceId) private, and exposed a trace id constructor (NewTraceID) which returns an interface object implementing an interface that offers the Encode() (string, error) method. That way, when the constructor is called, the serialization happens on the fly, and its result is stored in an internal field.

🦖

@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2023

Codecov Report

Merging #3019 (6f7f984) into master (14d80f6) will decrease coverage by 0.01%.
The diff coverage is 88.23%.

❗ Current head 6f7f984 differs from pull request most recent head ead38b6. Consider uploading reports for the commit ead38b6 to get more accurate results

@@            Coverage Diff             @@
##           master    #3019      +/-   ##
==========================================
- Coverage   77.00%   77.00%   -0.01%     
==========================================
  Files         228      228              
  Lines       17066    17057       -9     
==========================================
- Hits        13141    13134       -7     
+ Misses       3082     3081       -1     
+ Partials      843      842       -1     
Flag Coverage Δ
ubuntu 77.00% <88.23%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
js/modules/k6/experimental/tracing/client.go 69.44% <75.00%> (-1.36%) ⬇️
js/modules/k6/experimental/tracing/trace_id.go 86.66% <92.30%> (+7.71%) ⬆️

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@oleiade oleiade requested a review from codebien April 13, 2023 12:40
@oleiade oleiade requested a review from codebien April 14, 2023 08:25
@oleiade oleiade force-pushed the refactor/decouple-traceid-and-encoding branch 2 times, most recently from 5af642d to 472a0a7 Compare April 14, 2023 09:09
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

LGTM, just one comment

js/modules/k6/experimental/tracing/trace_id.go Outdated Show resolved Hide resolved
@oleiade oleiade force-pushed the refactor/decouple-traceid-and-encoding branch 2 times, most recently from 9353a00 to 6432fc5 Compare April 14, 2023 12:11
codebien
codebien previously approved these changes Apr 14, 2023
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

I think I was misunderstood.

As I go longer in comments - my main point was that the creation and the encoding are needlessly separated. There is nothing to do with a trace id apart from encoding it, so we can just get that as a single operation (newTraceID) that just returns the encoded traceid as a string. And forgo all the other types, methods and structs.

js/modules/k6/experimental/tracing/client.go Outdated Show resolved Hide resolved
js/modules/k6/experimental/tracing/trace_id.go Outdated Show resolved Hide resolved
@oleiade
Copy link
Member Author

oleiade commented Apr 24, 2023

You were misunderstood indeed @mstoykov 😄
Those are fair points 👍🏻 Dropping the Encode method indeed.

@oleiade oleiade force-pushed the refactor/decouple-traceid-and-encoding branch from d8e31a4 to ead38b6 Compare April 24, 2023 12:01
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM!

@oleiade oleiade merged commit e7b1c62 into master Apr 24, 2023
@oleiade oleiade deleted the refactor/decouple-traceid-and-encoding branch April 24, 2023 12:19
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.

4 participants