Experimental tracing module (1/3): implement trace ids generation and propagation #2853

oleiade · 2023-01-13T16:25:35Z

About

To keep Pull Requests as small as possible and reduce the review burden, this PR is the first in a chain of 3 implementing the decided API design for the experimental tracing module.

This first PR exposes the k6/experimental/tracing module to k6 scripts but does not include any public APIs. Instead, it implements the trace id generation and propagation logic according to the W3C trace contexts specification.

The code introduced in this PR will be used in further PRs by the Client implementation to produce a trace id to attach to a given request and emit as part of the output's data points metadata, as well as producing the trace context headers in the supported formats (W3C, B3, and Jaeger).

Much of this PR's code and logic is essentially inherited and refactored from the xk6-distributed-tracing project.

Looking forward to your review 🙇🏻

References

Next: #2854

js/modules/k6/experimental/tracing/module.go

codebien

Hey,
thanks for it 🙇 I left a few comments.

js/modules/k6/experimental/tracing/trace_id.go

codecov-commenter · 2023-01-16T14:58:01Z

Codecov Report

Merging #2853 (0da38e2) into master (41472d1) will decrease coverage by 0.17%.
The diff coverage is 75.35%.

❗ Current head 0da38e2 differs from pull request most recent head dbff8f9. Consider uploading reports for the commit dbff8f9 to get more accurate results

@@            Coverage Diff             @@
##           master    #2853      +/-   ##
==========================================
- Coverage   76.87%   76.70%   -0.17%     
==========================================
  Files         217      224       +7     
  Lines       16619    16843     +224     
==========================================
+ Hits        12776    12920     +144     
- Misses       3039     3091      +52     
- Partials      804      832      +28

Flag	Coverage Δ
ubuntu	`76.70% <75.35%> (-0.09%)`	⬇️
windows	`?`

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

Impacted Files	Coverage Δ
api/v1/status_routes.go	`58.33% <ø> (+25.68%)`	⬆️
js/timeout_error.go	`52.94% <0.00%> (-3.31%)`	⬇️
lib/execution.go	`89.90% <ø> (-2.76%)`	⬇️
lib/testutils/mockoutput/mockoutput.go	`41.66% <ø> (-4.49%)`	⬇️
js/modules/k6/experimental/tracing/client.go	`61.11% <61.11%> (ø)`
js/modules/k6/experimental/tracing/options.go	`63.63% <63.63%> (ø)`
output/manager.go	`79.16% <70.00%> (+4.16%)`	⬆️
execution/abort.go	`70.37% <70.37%> (ø)`
errext/abort_reason.go	`75.00% <75.00%> (ø)`
js/modules/k6/experimental/tracing/module.go	`75.55% <75.55%> (ø)`
... and 28 more

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

codebien

LGTM 👍

codebien

I'm not sure we should merge to master directly here, should we merge to a long-lived intermediate branch for aggregating the entire feature?

oleiade · 2023-01-16T16:06:12Z

I considered it indeed, especially as there are two other PRs have to be merged if this first sequence makes it on time for release. I'll create one preemptively, and if other reviewers agree, I would rebase the current PR train on it 👍🏻

mstoykov

LGTM!

I am glad we have smaller PRs.

In general it seems to me like 90% of this PR can be not under the js package. This likely will be useful in the future so that other js modules or parts of the system can use the tracing code.

This though likely should be done in a separate PR(s) as I don't want to add more things to be discussed for this PR chain.

Also I would've likely not introduced and registered the module in this PR but done it later.

Given that it seems like we agree to merge the other 2 PRs in this one and then merge this one - this is really not a problem that needs to be fixed. But is something I would like to note.

oleiade · 2023-01-19T14:25:01Z

Thanks a lot for your input @mstoykov; much appreciated. After giving it some thought, I agree with you; a lot of the code here could live in another, more reusable place in the repository.

I took note and will try it in a future PR during our cooldown phase.

I'm unsure where a valid alternative place for that code would be. Did you have something in mind? 💭 Maybe js/common somewhere in lib, or we create a new tracing or identifiers module somewhere?

oleiade · 2023-01-19T14:26:06Z

Heads-up @codebien and @mstoykov I've addressed the comments made so far, and also replaced the trace id's UnixTimestampNano with a time.Time as discussed in the second PR #2854 🙇🏻

codebien

The current code LGTM

Although we do not expose any of user-facing logic at this point, we register the "k6/experimental/tracing" module in k6 to prepare for further implementation of public APIs

This commit defines a Client type which exposes an API similar to the HTTP module, and allows users to perform HTTP requests embedding a trace-context, and attaching their trace_id as an output metadata to their HTTP-related samples. It uses the HTTP module's request function under the hood, and wraps it with some "hook" logic to ensure the expected headers are present, and that the expected output metadata are emitted. The tracing client exposes an option-set allowing to use the three available propagation format. Other options such as sampling, and baggage are present for forward-compatibility, but will be implemented at a later point in time.

This commit exposes the Client constructor publicly as part of the k6/experimental/tracing module. From this point forward users will be able to instantiate the Client, and perform instrumented HTTP requests using it. This commit also adds a bunch of integration tests covering the expected behavior of the module's API.

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

…trumentHTTP Experimental tracing module (3/3): define and expose a `instrumentHTTP` function

Experimental tracing module (2/3): define and expose a `tracing.Client`

samples/experimental/tracing-client.js

codebien · 2023-01-26T15:20:36Z

@oleiade can we get #2855 (comment) before merging this PR?

oleiade · 2023-01-26T15:26:37Z

@codebien considering it was brought up as a non-blocking request, I was planning to implement it in the PR implementing sampling (as I will have to modify the tests in the process anyways). If that's okay with you, I'd prefer to merge as is now 🙇🏻

codebien

LGTM

oleiade self-assigned this Jan 13, 2023

github-actions bot requested review from codebien and imiric January 13, 2023 16:25

oleiade added feature high prio labels Jan 13, 2023

oleiade added this to the v0.43.0 milestone Jan 13, 2023

This was referenced Jan 13, 2023

Experimental tracing module (2/3): define and expose a tracing.Client #2854

Merged

Experimental tracing module (3/3): define and expose a instrumentHTTP function #2855

Merged

oleiade requested a review from mstoykov January 13, 2023 16:56

oleiade mentioned this pull request Jan 13, 2023

Add experimental tracing js module #2838

Closed

mstoykov reviewed Jan 16, 2023

View reviewed changes

js/modules/k6/experimental/tracing/module.go Outdated Show resolved Hide resolved

codebien requested changes Jan 16, 2023

View reviewed changes

oleiade force-pushed the experimental/tracing-module-propagators branch from d0ef025 to 1b4a7a9 Compare January 16, 2023 14:57

oleiade force-pushed the experimental/tracing-module-propagators branch 3 times, most recently from 9df375a to beeacb4 Compare January 16, 2023 15:44

codebien previously approved these changes Jan 16, 2023

View reviewed changes

codebien reviewed Jan 16, 2023

View reviewed changes

mstoykov previously approved these changes Jan 18, 2023

View reviewed changes

oleiade dismissed stale reviews from mstoykov and codebien via 8b5b35c January 19, 2023 14:05

oleiade force-pushed the experimental/tracing-module-propagators branch from beeacb4 to 8b5b35c Compare January 19, 2023 14:05

oleiade requested review from codebien and mstoykov January 19, 2023 14:25

codebien reviewed Jan 20, 2023

View reviewed changes

mstoykov previously approved these changes Jan 20, 2023

View reviewed changes

Register a k6/experimental/tracing

3889364

Although we do not expose any of user-facing logic at this point, we register the "k6/experimental/tracing" module in k6 to prepare for further implementation of public APIs

oleiade force-pushed the experimental/tracing-module-propagators branch from 6e1c51b to 3889364 Compare January 23, 2023 15:03

oleiade added 2 commits January 23, 2023 16:03

oleiade mentioned this pull request Jan 24, 2023

Experimental tracing module (4/3): support for sampling #2874

Closed

oleiade and others added 13 commits January 24, 2023 14:55

fixup! return error instead of common.Throw in Client.Request

d5d3229

fixup! make counters in tests atomic

c2648ac

Expose the instrumentHTTP function as part of the module's public API

31d317d

Add integration tests covering the instrumentHTTP function

22f1f70

Update cmd/tests/tracing_module_test.go

2ff5246

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

Update cmd/tests/tracing_module_test.go

7f858e9

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>

Add unit tests for the instrumentHTTP function

7163425

Remove unnecessary integration tests

60e1e3b

Add integration test illustrating instrumentHTTP in multi-file setup

7d468aa

Move unit test setup in dedicated struct

d5616dc

Adjust test ensuring instrumenting function from other module works

7187e12

Merge pull request #2855 from grafana/experimental/tracing-module-ins…

f6ba5e4

…trumentHTTP Experimental tracing module (3/3): define and expose a `instrumentHTTP` function

Merge pull request #2854 from grafana/experimental/tracing-module-client

79c350d

Experimental tracing module (2/3): define and expose a `tracing.Client`

oleiade requested review from codebien and mstoykov January 26, 2023 14:12

codebien requested changes Jan 26, 2023

View reviewed changes

samples/experimental/tracing-client.js Outdated Show resolved Hide resolved

Move tracing-client example in the correct samples folder

dbff8f9

oleiade requested a review from codebien January 26, 2023 15:00

mstoykov approved these changes Jan 26, 2023

View reviewed changes

codebien approved these changes Jan 26, 2023

View reviewed changes

oleiade merged commit daa866a into master Jan 26, 2023

oleiade deleted the experimental/tracing-module-propagators branch January 26, 2023 15:38

imiric mentioned this pull request Feb 8, 2023

Add distributed tracing support #2128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimental tracing module (1/3): implement trace ids generation and propagation #2853

Experimental tracing module (1/3): implement trace ids generation and propagation #2853

oleiade commented Jan 13, 2023 •

edited

Loading

codebien left a comment

codecov-commenter commented Jan 16, 2023 •

edited

Loading

codebien left a comment

codebien left a comment

oleiade commented Jan 16, 2023

mstoykov left a comment

oleiade commented Jan 19, 2023

oleiade commented Jan 19, 2023

codebien left a comment

codebien commented Jan 26, 2023

oleiade commented Jan 26, 2023 •

edited

Loading

codebien left a comment

Experimental tracing module (1/3): implement trace ids generation and propagation #2853

Experimental tracing module (1/3): implement trace ids generation and propagation #2853

Conversation

oleiade commented Jan 13, 2023 • edited Loading

About

References

codebien left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 16, 2023 • edited Loading

Codecov Report

codebien left a comment

Choose a reason for hiding this comment

codebien left a comment

Choose a reason for hiding this comment

oleiade commented Jan 16, 2023

mstoykov left a comment

Choose a reason for hiding this comment

oleiade commented Jan 19, 2023

oleiade commented Jan 19, 2023

codebien left a comment

Choose a reason for hiding this comment

codebien commented Jan 26, 2023

oleiade commented Jan 26, 2023 • edited Loading

codebien left a comment

Choose a reason for hiding this comment

oleiade commented Jan 13, 2023 •

edited

Loading

codecov-commenter commented Jan 16, 2023 •

edited

Loading

oleiade commented Jan 26, 2023 •

edited

Loading