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

Meter name validation #3579

Closed
pellared opened this issue Jun 30, 2023 · 5 comments
Closed

Meter name validation #3579

pellared opened this issue Jun 30, 2023 · 5 comments
Assignees
Labels
spec:metrics Related to the specification/metrics directory

Comments

@pellared
Copy link
Member

pellared commented Jun 30, 2023

The current way the meter can be named may be unfriendly for Prometheus. For example refer to TestIncompatibleMeterName in open-telemetry/opentelemetry-go#4274. Prometheus exporter in OTel Go is logging the issue like this:

2023/06/30 14:09:31 cannot create scope info metric: label value "\xff\xfe\xfd" is not valid UTF-8

and the metric is not exported.

I just want to double-check if the existing behavior of OTel Go Prometheus exporter and OTel Specification is OK or if we should make the name validation more strict. E.g should the meter name validation be the same as for (or similar to) instrument name validation?

@pellared pellared added the spec:metrics Related to the specification/metrics directory label Jun 30, 2023
@pellared pellared changed the title Validate meter name in the same way as instrument name Meter name validation Jun 30, 2023
@pellared
Copy link
Member Author

@reyang, can you please take a look at this issue as I see that the instrument name validation was added as part of #1557?

@MrAlias
Copy link
Contributor

MrAlias commented Jul 26, 2023

I don't know if we can be more restrictive with the meter name and remain backwards compatible.

@pellared
Copy link
Member Author

pellared commented Jul 27, 2023

I don't know if we can be more restrictive with the meter name and remain backwards compatible.

It would be ABI backwards compatible as the API surface remains the same, but NOT behavioral backwards compatible as these are behavioral changes.

@MrAlias
Copy link
Contributor

MrAlias commented Jul 27, 2023

I don't know if we can be more restrictive with the meter name and remain backwards compatible.

It would be ABI backwards compatible as the API surface remains the same, but NOT behavioral backwards compatible as these are behavioral changes.

Right. I worry that a user that is currently using OTel will upgrade and instead of continuing to work their system starts erroring and not functioning. If we do decide to change the validation, I think the most we could do as a response would be to add log messages describing the error.

@jack-berg
Copy link
Member

Meter name manifests as scope name in the data model, and is akin to tracer name and logger name. For consistency, I think its important that the rules for scope name are the same across the signals.

I can't find anything in the data model that that suggests any limits around the characters or length of a scope name. Adding limits retroactively would definitely be a breaking change.

I get the argument that we could add rules and log when they're violated rather than erroring, but I see this is a prometheus exporter specific problem and not something that should influence tracer, meter, and logger more broadly. The crux is that OTLP to prometheus spec describes insufficient rules for mapping instrumentation scope to prometheus info-typed metrics. It appears that OpenTelemetry scopes have less constraints in terms of their allowable characters (and probably their length) than info typed metrics. Let's fix that problem rather than the much more disruptive and arguably breaking change of adjusting the rules of instrumentation scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:metrics Related to the specification/metrics directory
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants