-
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
[DRAFT] [mdatagen] Move attributes under each metric definition #16533
[DRAFT] [mdatagen] Move attributes under each metric definition #16533
Conversation
@djaglowski I would like to hear your opinion on this. If you agree on this approach, I'll move forward |
I like the overall goal of open-telemetry/opentelemetry-collector#10726 but I'm not sure that reworking attributes like this is an improvement. Is it technically necessary? My understanding is that it is not.
Solving this is the main goal right? If we remove confusion about the actual attribute name, then users can easily apply re-aggregation. Personally, I think that relying on anchors is less readable, both for developers and users. I'd much rather we find a solution that allows us to define shared attributes once. Could we just clarify the current attributes:
network_direction: # only used in metadata.yaml - no longer show in documentation
attribute_name: direction # required, formerly "value"
description: The direction of network data.
type: string
enum:
- received
- sent We can also improve the documentation of metrics in a similar way to what you've proposed: Metricselasticsearch.node.cluster.ioThe number of bytes sent and received on the network for internal cluster communication.
Attributes
elasticsearch.otherThe number of bytes sent and received on the network for something else.
Attributes
|
No, it is not.
I like your suggestion about making Also, I was thinking about a pretty similar rendering of documentation.md :) |
Seems reasonable to me. It's a simple improvement upon what we currently have. Changing the docs to be unambiguous about the name is the more important part of addressing user confusion. |
@djaglowski Sounds good. I've submitted the first PR to improve the generated documentation #16556 PTAL Will do values->override_name filed rename after that. Thanks for your feedback! |
Submitted another PR to rename the field #16561 . I belive it'll be a much better shape after that, so we can close this PR |
As part of open-telemetry/opentelemetry-collector#10726, I'd like to consider removing
attributes
section in metadata.yaml and defining attributes for each metric separately. The main reason for that is aligning the config interface for re-aggregation (enabling/disabling metric attributes) with metrics attributes representation in metadata.yaml. It also removes the confusion around figuring out what the actual attribute name is: whether it's a key name in theattributes
section or avalue
field in anattributes
item.On the other side, this move introduces a couple of complications:
<metric_name>Attribute<attribute_name><enum>
, this PR suggests a bit different approach based on interfaces that introduce a shorter formAttribute<enum>
. But the suggested approach may be seen as an excessive complication, so maybe better to keep<metric_name>Attribute<attribute_name><enum>
and have a constant for each enum in a metric attribute likemetadata.SystemCPUUtilizationAttributeStateSystem