-
Notifications
You must be signed in to change notification settings - Fork 21
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
Feat: Auto-label profiles based on request baggage #99
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! I've left a number of comments, but the main suggestion is to move the feature to a submodule (e.g., contrib/k6)
Another issue is that in Go The opentracing API does not allow for passing context in a general case, therefore there is no a trivial solution to this (if we want to reuse the tracing instrumentation for our purposes). As a workaround, we could "re-inject" the goroutine labels, but that would require binding to |
What I would also like to learn more about and discuss in detail is the integration scenario as a whole because I definitely lack the context. I'm wondering if the use of pprof labels is required at all: my understanding is that load tests never run concurrently on the test subject system (I just can't imagine when it may make sense), therefore it might be sufficient to refer to the time range for a given test run (identified with the series labels, such as |
In the interest of simplicity, its easier to keep all the logic of mapping baggage to labels contained within this library until such time that we find a need to provide users with more control over how baggage is mapped to labels.
contrib/go.mod
Outdated
@@ -0,0 +1,17 @@ | |||
module github.com/grafana/pyroscope-go/contrib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we go further and put k6
stuff into a submodule? contrib
would be just a container for integration sub-modules.
Otherwise, once we add another integration, its dependencies would be added to the k6 package, and vice-versa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! I agree with this move! I was attempting to do that last Friday but go work sync
was fighting with me. It kept trying to update the go version of other modules.
I'll continue to work on this though and get it addressed before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I got this fixed in https://github.com/grafana/pyroscope-go/pull/99/files/cbda95a5bdb83f75d53d811f4b921385e53e6ed0..016a1717a57831f63723fc46f4298531c17ac3f5. A couple things to note:
- I marked the
k6
package as experimental by moving it to thex/k6
submodule because this API may change or be removed entirely at any time. - I had to downgrade the go.opentelemetry.io/otel package from
1.27.0
to1.17.0
to make the submodule compatible with go versions <1.21.
Originally the k6 package was in the contrib/k6 submodule. However, it makes more sense to put it under x/k6 as it's still highly experimental and subject to change or removal at any point.
We inline this function call to help keep stack trace frames clean. This way, we avoid adding frames for `pyroscope.TagWrapper` and `pprof.Do`.
@kolesnikovae As you take another look, note that I moved the code into the I tested this code on a local instance of pyroscope and it works well: We get all the k6 labels mapped correctly: That said, we need to modify the upstream k6 js library to be more pragmatic about the |
In order to support integrations with k6 load testing, we need to be able to pull key-value pairs out of the request baggage and create labels to add to the profile for that request.
Instead of targeting specifically k6 baggage, I create a semi-extensible feature to attach arbitrary labels from the request baggage.Edit It made more sense to be opinionated about how we go from baggage to labels. The package no longer allows users to define arbitrary filter and transformation rules, rather we will only select baggage prefixed withk6.
to be mapped to labels. This significantly simplified the API.To use this feature in conjunction with k6, the
LabelsFromBaggageHandler
should wrap the routes that should have this auto-labeling behavior (ideally it wraps the http server mux to enable it for all handlers), like so:This example adds auto-labeling to all the routes (
/
,/bike
,/scooter
,/car
) for any request baggage with k6-specific members.All of this code lives in the
github.com/grafana/pyroscope-go/x/k6
module.x/k6
was specifically chosen because this API is experimental. We may chose to modify it or outright remove it at any time. Therefore, it made sense to place it in this package to signal the potential instability of the API. As we test this package in production environments and are satisfied with it, we can move it tocontrib/k6
to make it an official, stable extension.