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

Metric naming conventions reasoning #3207

Open
humivo opened this issue Mar 1, 2023 · 8 comments
Open

Metric naming conventions reasoning #3207

humivo opened this issue Mar 1, 2023 · 8 comments
Assignees
Labels
bug Something isn't working metrics

Comments

@humivo
Copy link
Member

humivo commented Mar 1, 2023

Describe your environment I am currently using the Python SDK for a sample app that creates metrics instruments like Counter, UpDownCounter, and etc. When my metrics were exported to Cloudwatch, I noticed that the metric names are all lowercase so I checked the SDK and found here that metric names are converted to all lowercase for some reason. I was wondering why the metric names are converted to lowercase, and is this the naming convention we should follow? If so, should this be documented somewhere? Thank you.

@humivo humivo added the bug Something isn't working label Mar 1, 2023
@srikanthccv
Copy link
Member

They are case-insensitive, ASCII strings.

It is part of the metrics specification https://github.com/open-telemetry/opentelemetry-specification/blob/ec84e5dcd77f36e79c0fe9bb1444d62be79dbb38/specification/metrics/api.md#instrument-name-syntax

@bryan-aguilar
Copy link

bryan-aguilar commented Mar 2, 2023

Does case insensitive imply that fOo should be converted to foo or that fOo should be considered the equivalent to foo? I don't believe other metric sdks force instrumentation names to lowercase before exporting.

My point being is that fOo is semantically correct since they are case insensitive right? Wouldn't an end user expect the SDK to export the metric with the same case as they defined?

@ocelotl
Copy link
Contributor

ocelotl commented Mar 3, 2023

Does case insensitive imply that fOo should be converted to foo or that fOo should be considered the equivalent to foo? I don't believe other metric sdks force instrumentation names to lowercase before exporting.

That is not explicitly specified in the spec so there are different ways of implementing. Java does this, which means it ignores the case when aggregating metrics from instruments with equivalent names: if foo and Foo are registered in that order, their data will be aggregated together and the output instrument name is the one registered first (@jack-berg please correct me if I am wrong here).

My point being is that fOo is semantically correct since they are case insensitive right? Wouldn't an end user expect the SDK to export the metric with the same case as they defined?

Yes, fOo or Foo are both semantically correct. Maybe the user would expect the SDK to export the metric with the same name, maybe the user would expect the second instrument to override the first one. That is just not specified.

@srikanthccv @lzchen @aabmass I think we are being compliant with what is being specified here, WDYT?

@bryan-aguilar
Copy link

From personal experience on our team we were caught off guard when we found out that FOO exports as foo. We were trying to query for FOO in cloudwatch. It seems to me that a user would expect to have the case they defined be the one that is exported. At the very least this should be clearly called out in the documentation. Is it and we were unable to find it?

@srikanthccv
Copy link
Member

The metrics API/SDK specification left room for implementation flexibility and interpretation. It says

It is unspecified whether or under which conditions the same or different Instrument instance will be returned as a result of duplicate instrument registration.

The Python SDK implementation returns the same instrument if the metric identity if already seen

The term identical applied to Instruments describes instances where all identifying fields are equal.

And it helps us with the

Based on the recommendations from the data model, the SDK MUST aggregate data from identical Instruments together in its export pipeline.

We can probably export as-is when only one instrument is registered. So what do you do when duplicate instruments are registered? There are multiple options here

  1. lowercase them all
  2. use the first registered instrument name
  3. use the last duplicate instrument name (someone might expect to override)
  4. more?

Since it's not explicitly specified, any option is acceptable IMO, and SDK chose the first regardless of the duplication. If a backend says it supports OTLP protocol, should it also keep the case-insensitive search?

@humivo
Copy link
Member Author

humivo commented Mar 7, 2023

Thank you for the explanation as it makes a lot of sense and is very helpful. Due to the SDK lowercasing the instrument names in case of duplicate instrument handling, is it recommended to use all lowercase instrument names in the first place so we know what to expect?

@srikanthccv
Copy link
Member

I would suggest doing that. I am also open to the idea of SDK using the case of the first registered instrument in case of duplicates. And I think it should be technically possible to create two instruments with the same name but different casing and then use views to modify the resulting metric name.

When a duplicate instrument registration occurs, and it is not corrected with a View, a warning SHOULD be emitted.

I believe this is the issue created to address that #2558.

@lzchen
Copy link
Contributor

lzchen commented Sep 28, 2023

As a result of this discussion, we have decided to remove the lowering of metric names and respect casing for instrument names. Similar to js, we will be treating this as a bug fix even though technically it involves behavioral breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working metrics
Projects
None yet
Development

No branches or pull requests

5 participants