-
Notifications
You must be signed in to change notification settings - Fork 1
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
Update histograms code, move AO metrics exporter to main package #458
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.
Now i see the very cool OTel-to-AO metrics export you mentioned! One concern is tags (cardinality) explosion for AO.
const scopeTags = this.#scopeTags(scopeMetric.scope) | ||
|
||
for (const metric of scopeMetric.metrics) { | ||
const name = `trace.node.${metric.descriptor.name}` |
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.
This means OTel-originated metrics that get exported via apm-proto to AO will all be prefixed with trace.node.*
(similar to runtime metrics)?
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 ! That's the standard they've been following
const [tags, tagCount] = this.#oboeTags({ | ||
...scopeTags, | ||
...descriptorTags, | ||
...dataPointTags, |
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.
Does this mean all the above are set as the AO metric tags? Not sure what's a typical number of OTel metrics attrs to expect, but out of paranoia would it be easy to cap the max number of tags to 50 (inspired by https://github.com/solarwinds-cloud/trace/blob/master/docs/specs/custom-metrics.md) just to not rock the AO boat too much.
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.
Good point, I added an upper limit.
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, thanks @raphael-theriault-swi!
This histogram code is ported from ingestion, the exporter code is just copied from the existing one.