Conversation
| // capture the original values late, so that they include any previously made instrumentation changes | ||
| const currentRequest = http.request; | ||
| const currentAsyncRequest = http.asyncRequest; | ||
|
|
||
| const client = new Client(null, currentRequest, currentAsyncRequest); |
There was a problem hiding this comment.
This is the main change compared to the previous version. We capture http.request and http.asyncRequest during the instrumentation phase to make sure we use the latest version of their methods.
| // capture the original values late, so that they include any previously made instrumentation changes | ||
| const currentRequest = http.request; | ||
| const currentAsyncRequest = http.asyncRequest; | ||
|
|
||
| const client = new Client(opts, currentRequest, currentAsyncRequest); |
There was a problem hiding this comment.
Similar to the above, this is the main change compared to the previous version.
|
This would fix the concrete problem, but does introduce now a ... different problem, which might be less problematic in a real situation. But first - please remember to update the docs in https://grafana.com/docs/k6/latest/javascript-api/jslib/http-instrumentation-tempo/client/ and for pyroscope. This changes also mean that to use the Client you now need to provide the request and asyncRequest parameters which make it harder to use. I do expect most users to potentially not use that , but still. But the bigger problem is that now calling instrumentHttp any time after the first time will not have any effect. Well it will call more functions, but then the header will be overriden by the first caller. I do not know if anyone is using this to send different stuff or to change the sampling rate (the more likely case). But if it isn't going to work, I would propose that we actually throw and exception on second call 🤷 This will also get more complicated if you use both and you call instrumentHTTP for both tempo and pyroscope and then you want ot override one or the other in some different orders. |
|
@mstoykov, these are fair points! I've pushed changes to retain current signatures of the Regarding calling |
andrewslotin
force-pushed
the
make_tempo_and_pyroscope_compatible
branch
from
8af00dd to
139339c
Compare
| // event params would be nullish, we'll instantiate | ||
| // a new object. | ||
| if (args[1] == null) args[1] = {} | ||
| args[1].headers = mergeHeaders(args[1].headers || {}, headers) |
There was a problem hiding this comment.
Another fix to preserve the existing baggage header values. Instead of overriding, we just merge two sets of key-value pairs.
|
@mstoykov, PTAL, I've updated Pyroscope instrumentation to preserve |
|
I am not certain merging baggage is good idea, but I guess if instrumentHttp is nto supposed to be callable multiple times, it wouldn't really matter. I do think we probably should split this in a new repo if we are going to do more changes to it so we can better track changes. I had to check this out locally and then look at diffs locally between files. In this case arguably the change is big enough that this was worth it , but for some small change it seems a bit much. |
3 participants
Description
This PR introduces new versions for Tempo and Pyroscope instrumentation libraries that are compatible with each other. The core change is that
http.requestandhttp.asyncRequestin both libraries are now being captured at during theinstrumentHTTP()call instead of during the import phase, which ensures that instrumented code wraps the latest, potentially already patched version of the method.Update: following the discussion with @mstoykov I've added another fix for the Pyroscope instrumentation library, that now preserves the values of
baggageheader provided torequestand appends k6 metadata to it instead of overriding.Closes #151
npm run testcommand succeeds.yarn run generate-homepagelocally and verify the new homepage/lib/index.htmlfile looks legit.version bumplabel./lib/{jslib_name}/{desired_version}folder./lib/{jslib_name}/{desired_version}/index.jsfile containing the jslib's code bundle.supported.jsonfile to contain an entry for the newly added jslib version/tests/basic.jsand/tests/testSuite.jsfiles to ensure that the new version of the jslib is importable and runnable by k6.