search

open-telemetry / opentelemetry-python

OpenTelemetry Python API and SDK
https://opentelemetry.io
Apache License 2.0
1.71k stars 599 forks source link

Metric naming conventions reasoning #3207

Open humivo opened 1 year ago

humivo commented 1 year ago

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.

srikanthccv commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/open-telemetry/opentelemetry-python/issues/2558.

lzchen commented 11 months ago

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.