-
Notifications
You must be signed in to change notification settings - Fork 577
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
Traces get rejected if labels have dashes in them #3359
Comments
Hi @kbolashev! Thank you for filing the issue. We're currently using prometheus data model for labels (including those that are set as pprof tags) for consistency with other products:
Recently, we've relaxed the requirement, by allowing Before UTF-8 labels are fully supported in Prometheus, I'm a little hesitant to add more exceptions. Instead, I'd consider altering how we handle such labels: instead of discarding the profile, we could drop the invalid labels. What do you think? I should note that the use of high cardinality labels will very likely cause performance issues. For example, if you set the PID of the spawn processes as a pprof label, you will get a new profile entry in the storage for each such process. If you wouldn't add this label to Prometheus metrics, you wouldn't want to add it as a pprof label either.
Out of curiosity, is this a publicly available package? |
Honestly this would totally work for us, because being able to see the goroutines at all already gets much further and we don't really care about drilling down deeper for now.
Yes, sure! We're reusing some of Gitea's code for parsing git repos, and they seem to have their own in-house profiling. https://github.com/go-gitea/gitea/blob/main/modules/process/manager.go#L29
Thanks for the warning on this! We'll keep track at how it performs and whether the performance degrades too much for us. |
Thank you for the input, @kbolashev! We'll look into this soon (I'm sorry, I don't have a clear ETA).
Yeah, or scaling the Pyroscope deployment if performance degrades badly and the labels are really needed |
Describe the bug
Hi Pyroscope team, thank you very much for this great tool!
We're trying to integrate it into our stack to profile some Go code, but hitting problems with ingesting goroutine and cpu traces.
Some of dependency code we're using that calls subprocesses for execution, sets pprof labels on goroutines to track these subprocesses (PID, etc.). These labels have dashes in them (e.g.
process-description
), which seems to conflict with Pyroscope's ingestion and we're failing to receive CPU and Goroutine traces (memory traces work fine).I found the validation code, assuming I'm hitting this line. I'm wondering, if there's a way to skirt by this validation, or alternatively maybe sanitize these at
pyroscope-go
level.To Reproduce
Steps to reproduce the behavior:
http://localhost:4040
Expected behavior
All traces show up in pyroscope
Actual: the cpu and goroutine traces don't show up,
Environment
Additional Context
Error in the stdout:
The text was updated successfully, but these errors were encountered: