-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[metricbuilder] proposing adding NewMetricData #8255
[metricbuilder] proposing adding NewMetricData #8255
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.
I think this is a great idea and looks good.
Just one comment on a complementary change.
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, though we should have a changelog entry for the changes to the instrumentation library names.
Many metric scrapers are setting the instrumentation name after initializing pdata. This change proposes adding a generated function that could be used more generically instead. It pulls the instrumentation name from the metadata for consistency.
3122208
to
62ccaa4
Compare
@djaglowski i added a note to the changelog. Any thoughts on whether the right thing to do is to name receivers: Additionally, the scrapers for various host metrics end up with |
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.
@codeboten, I don't feel strongly about it either way, but the more verbose otelcol/redisreceiver
,otelcol/hostmetrics/cpu
seem slightly clearer to me.
Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>
I agree with @djaglowski. |
@dmitryax @djaglowski for consistency the hostmetrics scrapers are now identified with: I'll follow this PR with another for different components to use |
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
Description:
Many metric scrapers are setting the instrumentation name after initializing pdata. This change proposes adding a generated function that could be used more generically instead. It pulls the instrumentation name from the metadata for consistency.
Note this PR is updating the code in a few scrapers to show what the change looks like. I'm happy to split that out of this PR if folks are interested in moving forward with this proposal.
Testing: unit tests