Add tests ensuring the tracing module sets and cleans up metadata properly #2894
Conversation
8999740 to
920bb59
Compare
Codecov Report
@@ Coverage Diff @@
## master #2894 +/- ##
==========================================
- Coverage 77.03% 76.98% -0.06%
==========================================
Files 228 228
Lines 17059 17068 +9
==========================================
- Hits 13141 13139 -2
- Misses 3076 3086 +10
- Partials 842 843 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 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. |
There was a problem hiding this comment.
LGTM!
I was thinking more of js code test that has
callToTestMetadata()
http.post(...)
callToTestMetadata() maybe something like what I do in the async group code but with a bertter name, The script that is run is in test case sabove the line I linked.
Goja's author also does something similar in https://github.com/dop251/goja/blob/8a0df7296ebdd6b66ab2bb8230f9744dc8824825/func_test.go#L212
|
@mstoykov that's another clever way of doing it indeed 🤝 When looking at how to access the metadata before the callback is called, I didn't think of doing it from the JS indeed. In this case, I find the unit test ends up being "lower-context", forcing me to make the code a bit more flexible by using interfaces. If you have a strong opinion towards making it in JS, I could give it a shot too. Otherwise, I would keep it as is as unit tests and the JS approach in mind for the next time. Let me know 😸 |
|
I have re-run the test-tip check to verify it is just one of the flaky. |
There was a problem hiding this comment.
Having now gone through the code one more time and through #2886 it seems like the new Producer only purpose is to make the current test possible.
There is also no "pruducer" abstraction or terminology within the tracing vocabulary as well.
Given just how much code this adds and that I feel like it makes everything harder and the fact this all can be tested with just a single defintion of a helper js function as I said before - I would prefer we go that route and this PR to only add tests instead of also refactor.
Additionally this will hopefully make this PR have less conflicts with the sampling one
9362103 to
520289d
Compare
|
@mstoykov Although I disagree with the perceived complexity introduced by the approach in this PR, I see your point, and I'm indeed open to doing it in JS. After experimenting with your examples and approach, I want to clarify one of my findings. Unless I missed something, as opposed to my current approach, testing in JS won't let me assert that an instrumented function follows the underlying steps of:
Unless I missed or misunderstood something, what I can test from JS is only that the |
|
tl;dr I checked out the branch to test something and got carried away :(
The problem for me is that all the new changes exist to make it possible to test this (slightly) better. As you can see from my try at this the only thing I am not checking is the concrete value of the trace_id. I could be testing its format, but decided to not spend more time on it. While I also worked on this I realized I also have an issue with the fact you are basically having a unit test, while I would argue that we need more of an integration test. One in which you do what the user will do and check that we do set the metadata correctly and then unset it after the operation. But doing this with actual JS code as the user will use it instead of what seems like a reimplementation of the actual code the user will call. Both have their place, but arguably we care more about the user experience instead of the underlying implementation details. The "integration" test here did take more fiddling in some parts due to the existence of two functions generating test setups, but mostly due to the requirements of the k6/http package so that it is actually functional. But even with that both commits are on par with just the code that is to add the Producers and add the mock ones. This also now lends itself to be expanded so we can add more tests directly in the package instead of adding it to actual integration tests https://github.com/grafana/k6/tree/master/cmd/tests |
|
@mstoykov Thanks for the thorough examples, I see what you meant much better now indeed. I'm grateful for it 🙇🏻 I hadn't consider the |
7b6265c to
a85227e
Compare
|
I've updated the PR according to your recommendations. I've integrated the tests you kindly put together Mihail, thanks again for that 🤝 I've also dropped the |
|
@oleiade the CI reported a panic |
|
👍🏻 @codebien looking into it |
a85227e to
505915b
Compare
8756d59 to
2532a35
Compare
In order to be able to test how the client updates VUs metadata with a trace_id, we need to be able to predictably control the generated trace ids. Because the traceID encodes itself, we set a private an `io.Reader` randomness source attribute on it that will be used during encoding. That way we can for instance pass a reader that always produces the same value in the context of tests.
2532a35 to
65727a4
Compare
e79f122 to
0f9f0b5
Compare
0f9f0b5 to
8083ef7
Compare
|
@mstoykov done 👍🏻 |
| func (c *Client) instrumentedCall(call func(args ...goja.Value) error, args ...goja.Value) error { | ||
| if len(args) == 0 { | ||
| args = []goja.Value{goja.Null()} | ||
| } | ||
|
|
||
| traceContextHeader, encodedTraceID, err := c.generateTraceContext() | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
nit: this Pr would've been easier to code review if you didn't move this methods around in the codebase
There was a problem hiding this comment.
Fair enough 🙇♂️Sorry for that. I'm trying to get better at isolating these kind of changes, but still having a hard time.
This Pull Request addresses #2889, and adds tests ensuring that the tracing module does add a
trace_idmetadata entry when performing a request and cleans it up afterward.To make it possible, I had to wrap a bunch of existing behaviors throughout the module in interfaces. That way I could easily swap them with mocked/controllable behaviors: trace id generation and randomness generation.
Let me know what you think 🤝
closes #2889