Skip to content
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

add additional attributes to metric #5453

Closed
wants to merge 2 commits into from

Conversation

oliver-zhang
Copy link
Contributor

Sometimes, we need to add extra properties to the metrics, but I can't add it via auto Instrumentation,so I define a SPI ConfigurableMetricAttributesProvider, it returns Attributes, you can implement it yourself, When registering the view, we call the SPI interface to get the extra attributes and then add it to the AttributeProcessor

@oliver-zhang oliver-zhang requested a review from a team May 15, 2023 10:10
ConfigurableMetricAttributesProvider.class)){
attributesBuilder.putAll(provider.addCustomAttributes(configProperties));
}
SdkMeterProviderUtil.appendAdditionalAttributes(builder, AttributesProcessor.append(attributesBuilder.build()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is an extension to the YAML file configuration for views, I think it would make more sense to implement this as an additional section of key value pairs within the YAML, rather than an SPI that surprisingly interacts with all registered views.

With that said, I'm reluctant to add configurable surface area for adding these attributes since using views to add additional attributes is experimental and not an official part of the spec. That means that when we replace this file based YAML config with the full file configuration solution (currently being worked on), we'd have to drop the ability to append attributes via views.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is an extension to the YAML file configuration for views, I think it would make more sense to implement this as an additional section of key value pairs within the YAML, rather than an SPI that surprisingly interacts with all registered views.

With that said, I'm reluctant to add configurable surface area for adding these attributes since using views to add additional attributes is experimental and not an official part of the spec. That means that when we replace this file based YAML config with the full file configuration solution (currently being worked on), we'd have to drop the ability to append attributes via views.

Agree, My earliest consideration was the implementation you mentioned. Perhaps a specific metric needs to extend attributes,that's very suitable,Considering the common attributes of metrics, such as clusters, planes, etc., If each metric needs to be configured separately, it would be a lot of work,So I adopted the SPI. Do you have any good suggestions for the common attributes of all metrics, There maybe be better way. @jack-berg

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mentioned this in slack, but the most correct solution is probably to create a delegating MetricExporter which enriches MetricData with the additional attributes before calling a subsequent MetricExporter. While probably the most correct solution, this is awkward because it requires you to provide your own implementations of the MetricData interfaces. There's an issue tracking this here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants