-
Notifications
You must be signed in to change notification settings - Fork 318
Add design doc for Go labels #465
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
Conversation
Co-authored-by: Felix Geisendörfer <[email protected]>
- When disabled custom labels has little to no impact on performance or memory usage of the profiler | ||
- Custom labels should be limited so that even if a program has thousands of eligible labels the number supported is reasonably small (mostly enforced by eBPF itself) | ||
- Custom labels should be short and have fixed memory overhead | ||
- The custom labels should be made available to the reporter backend but otherwise it should be left up to implementors what to do with them |
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.
To fit into OTel it would be great to make the costum labels available with the OTel SemConv. Package reporter should just be an implementation detail.
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.
How does one make something available with the OTel SemConv package?
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.
Taking environment variable as example, information like labels can be provided as attribute:
opentelemetry-ebpf-profiler/reporter/internal/pdata/generate.go
Lines 230 to 233 in afbda36
attrMgr.AppendOptionalString( | |
sample.AttributeIndices(), | |
attribute.Key("env."+key), | |
value) |
From a quick lock, currently there is no specific OTel SemConv for labels afaict. But we should use a label that is commonly agreed until there is a OTel SemConv variant. Where this label key/value pair is used, e.g. in package reporter, then just becomes an implementation detail.
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.
So should I add something like this to the custom labels PR:
https://gist.github.com/gnurizen/1b2c6bd8904a603d8ed621d57e28c83a
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.
Just some minor nits. The overall idea sounds good to me.
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 also merge this now since the PR got merged? @gnurizen if there's follow up work that includes changes to this design document we can open a new PR.
No description provided.