-
Notifications
You must be signed in to change notification settings - Fork 487
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
pyroscope.scrape: Standarize profile names #6367
pyroscope.scrape: Standarize profile names #6367
Conversation
When a user adopts godeltaprof_, currently the resulting profile type name is shown separately from the default ones. That makes is harder than it should be to adopt godeltaprof. I am proposing that we send the profiles still under the name without the godeltaprof prefix.
3aa265d
to
62d98c8
Compare
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.
LGTM personally, if @korniltsev is OK with it then we can merge.
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.
LGTM with some notes
There is a piece of code determining if we should compute delta based on labels
https://github.com/grafana/agent/blob/6eb188959fe1dafda56ed34a8fb7c7fb9832ec99/component/pyroscope/scrape/scrape_loop.go#L188-L187
It looks like it is using allLabels
, not the modified publicLabels
so we should be ok there. I would maybe write a test to make sure delta appender is never used for godeltaprof renamed targets.
Another issue we could have is hash collision in case when there are 2 targets with memory and godeltaprof memory, the hash is calculated on public labels and they may end up the same after renaming. Not sure what to do best here. Maybe do not allow scraping godelttaprof_memory and memory at the same time from the same component?
This PR has not had any activity in the past 30 days, so the |
This is now covered by: grafana/alloy#662 |
When a user adopts godeltaprof_, currently the resulting profile typename is shown separately from the default ones. That makes is harder than it should be to adopt godeltaprof.
I am proposing that we send the profiles still under the name without the godeltaprof prefix.
@korniltsev do you think this might break anything down the line? I don't think so
@aknuds1 reported that initially