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

[connectors/spanmetrics] Rename the operation attribute to span.name #18529

Closed
kovrus opened this issue Feb 13, 2023 · 9 comments
Closed

[connectors/spanmetrics] Rename the operation attribute to span.name #18529

kovrus opened this issue Feb 13, 2023 · 9 comments
Labels
connector/spanmetrics needs triage New item requiring triage processor/spanmetrics Span Metrics processor

Comments

@kovrus
Copy link
Member

kovrus commented Feb 13, 2023

Component(s)

connector/spanmetrics

Describe the issue you're reporting

It should bring data point attributes produced by the spanmetrics connector closer to the specification, see Span spec.

It is the change that will be done in the new (not yet enabled) component, so it does not break anything for the existing users. However, we have to inform users about these changes when migrating from spanmeterics processor to spanmeterics connector.

@kovrus kovrus added the needs triage New item requiring triage label Feb 13, 2023
@github-actions github-actions bot added the processor/spanmetrics Span Metrics processor label Feb 13, 2023
@github-actions
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@albertteoh
Copy link
Contributor

I think it's good, in principal, to bring this dimension name closer to the specification; so I have no objections.

Note that if we agree to rename operation to span.name, it will be a breaking change, which will also impact Jaeger SPM.

@kovrus
Copy link
Member Author

kovrus commented Feb 14, 2023

@albertteoh Maybe we can do this change in the new component spanmetrics connector component and mark operation as deprecated in the spanmetrics processor component? Also, we can drop _total from the metric name in the spanmetrics connector. So we won't do any breaking changes in the processor now and will give users some time to migrate to the new spanmetrics connector that has these breaking changes. WDYT?

@Frapschen
Copy link
Contributor

I recommend the metric name in the new 'spanmetric' can follow Prometheus's best practices. Giving a prefix to the metric name and keeping the _total.

@kovrus
Copy link
Member Author

kovrus commented Feb 14, 2023

I recommend the metric name in the new 'spanmetric' can follow Prometheus's best practices. Giving a prefix to the metric name and keeping the _total.

The new spanmetric connector is the OTel component (not Prometheus specific component), by keeping _total as the metrics name we mix OTel data model and its naming convention with Prometheus conventions. I think, Prometheus's best practices have to be respected when generated metrics will be exported, e.g. with prometheusremovewrite and prometheus exporters (and that is already implemented). If generated metrics are exported with some other vendor or not vendor-specific component they should respect probably some other practices.

See also discussion here.

@albertteoh
Copy link
Contributor

@albertteoh Maybe we can do this change in the new component spanmetrics connector component and mark operation as deprecated in the spanmetrics processor component? Also, we can drop _total from the metric name in the spanmetrics connector. So we won't do any breaking changes in the processor now and will give users some time to migrate to the new spanmetrics connector that has these breaking changes. WDYT?

Great suggestion, and good timing with the new spanmetrics being recently merged! 👍🏼

@Cluas
Copy link
Contributor

Cluas commented Feb 15, 2023

The current spanmetrics connector implementation is just a port, can we add a config like standardized, Cluas@4530423

@github-actions
Copy link
Contributor

Pinging code owners for connector/spanmetrics: @albertteoh. See Adding Labels via Comments if you do not have permissions to add labels yourself.

@kovrus kovrus changed the title [processor/spanmetrics] Rename the operation attribute to span.name [connector/spanmetrics] Rename the operation attribute to span.name Feb 15, 2023
@djaglowski
Copy link
Member

Closed by #18746

@kovrus kovrus changed the title [connector/spanmetrics] Rename the operation attribute to span.name [connectors/spanmetrics] Rename the operation attribute to span.name Feb 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
connector/spanmetrics needs triage New item requiring triage processor/spanmetrics Span Metrics processor
Projects
None yet
Development

No branches or pull requests

5 participants