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

Allow metric instrument names to contain / characters #3422

Closed
aabmass opened this issue Apr 20, 2023 · 16 comments · Fixed by #3684
Closed

Allow metric instrument names to contain / characters #3422

aabmass opened this issue Apr 20, 2023 · 16 comments · Fixed by #3684
Assignees
Labels
spec:metrics Related to the specification/metrics directory

Comments

@aabmass
Copy link
Member

aabmass commented Apr 20, 2023

What are you trying to achieve?

Create instruments/metrics with names like composer.googleapis.com/environment/database/auto_failover_request_count:

  • containing / characters
  • exceeded 63 characters.

These types of metrics are very common in Google Cloud Monitoring (examples, notice the domain prefix is part of the metric) and we would like it to be possible to create such metrics with OTel instrumentation.

What did you expect to see?

It should be possible to create such an instrument e.g. in Python

from opentelemetry.sdk.metrics import MeterProvider
MeterProvider().get_meter("foo").create_counter("composer.googleapis.com/environment/database/auto_failover_request_count")

However it fails with

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/local/google/home/aaronabbott/repo/opentelemetry-js/venvtmp/lib/python3.10/site-packages/opentelemetry/sdk/metrics/_internal/__init__.py", line 97, in create_counter
    instrument = _Counter(
  File "/usr/local/google/home/aaronabbott/repo/opentelemetry-js/venvtmp/lib/python3.10/site-packages/opentelemetry/sdk/metrics/_internal/instrument.py", line 56, in __init__
    raise Exception(_ERROR_MESSAGE.format(name))
Exception: Expected ASCII string of maximum length 63 characters but got composer.googleapis.com/environment/database/auto_failover_request_count

As required by the spec

  • Subsequent characters must belong to the alphanumeric characters, '_', '.', and '-'.
  • They can have a maximum length of 63 characters.

Additional context.

Looks like this was introduced in #1557 by @reyang and the oldest reference I could find to these requirements was in #601 by @jmacd.

@aabmass aabmass added the spec:metrics Related to the specification/metrics directory label Apr 20, 2023
@MrAlias
Copy link
Contributor

MrAlias commented Apr 20, 2023

I'm pretty sure the original restriction is to ensure compatibility with Prometheus. If we drop the restriction it could mean our instrument names cannot be converted to Prometheus data.

@tsloughter
Copy link
Member

Except for the length, right?

Might be better to open a separate issue about the 63 character length.

@reyang
Copy link
Member

reyang commented Apr 20, 2023

@aabmass here is my suggestion to move this forward:

  1. List out the options you could think of.
  2. Provide your suggestion, including which option you would suggest, what's the plan (e.g. if we "just remove the length restriction", it could break existing exporters/backends).

@aabmass
Copy link
Member Author

aabmass commented Apr 20, 2023

I'm pretty sure the original restriction is to ensure compatibility with Prometheus. If we drop the restriction it could mean our instrument names cannot be converted to Prometheus data.

We already allow . (used extensively in semantic conventions) and - which prometheus doesn't allow. The compatibility spec says to convert any invalid characters to _ which we already do in this case.

I believe someone from OTel was in contact with Prometheus folks about trying to allow additional characters.

@aabmass
Copy link
Member Author

aabmass commented Apr 20, 2023

2. what's the plan (e.g. if we "just remove the length restriction", it could break existing exporters/backends).

I don't think Prometheus has any length restrictions so collector exporters may already receive such metrics from a Prometheus receiver and probably others.

I agree it could break SDK exporters but I'm not aware of any that would run into this issue. We could definitely open it up and provide an option to truncate like we do for attribute value limits. Do you have any specific SDK exporters in mind?

@reyang
Copy link
Member

reyang commented Apr 21, 2023

Do you have any specific SDK exporters in mind?

Nope. I haven't spent time looking into the existing exporters.

@jmacd
Copy link
Contributor

jmacd commented Apr 25, 2023

I support increasing the size limit. I worry that like dots, the slash character is not supported by Prometheus and it would be nice to investigate whether there are any technical reasons why Prometheus could not change their character set. In the case of dot, we could not find a technical reason.

@lmolkova
Copy link
Contributor

lmolkova commented Apr 25, 2023

Related: open-telemetry/opentelemetry-dotnet-contrib#740 - .NET emits its own counters with names that don't fit into 63 characters and have to be abbreviated or truncated to meet the limit, which seems quite small and arbitrary.

@tsloughter
Copy link
Member

I said in the spec meeting I found that Prometheus limited to 255. I was wrong :). What I actually found so far is librato and new relic which limit to 255 https://www.librato.com/docs/kb/faq/best_practices/naming_convention_metrics_tags/ and https://forum.newrelic.com/s/hubtopic/aAX8W0000008XqUWAU/maximum-length-of-a-metric-name

@trask
Copy link
Member

trask commented May 12, 2023

we have also had problems with the length limit in Java: open-telemetry/opentelemetry-java-instrumentation#6300

@trask
Copy link
Member

trask commented Jun 8, 2023

we continue to get bug reports in Java related to the length restriction: open-telemetry/opentelemetry-java-instrumentation#7448 (comment)

@reyang
Copy link
Member

reyang commented Aug 14, 2023

Update - I've also got folks asking for names that contain (SPC) character.

@B3zaleel
Copy link

Related: open-telemetry/opentelemetry-dotnet-contrib#740 - .NET emits its own counters with names that don't fit into 63 characters and have to be abbreviated or truncated to meet the limit, which seems quite small and arbitrary.

@lmolkova Counters that exceed that limit are disabled and are usually not exported.

@aabmass
Copy link
Member Author

aabmass commented Aug 29, 2023

Discussed again in the SIG today. We couldn't find a strong reason against allowing / and it is needed to be compatible with OpenCensus SDKs. I will send a PR.

@aabmass aabmass changed the title Allow metric instrument names to contain / characters and exceed 63 characters long Allow metric instrument names to contain / characters Sep 7, 2023
carlosalberto pushed a commit that referenced this issue Sep 22, 2023
## Changes

Updated the following entry in the spec compliance matrix:

* Section Metrics, item "Instrument names conform to the specified
syntax."
* Language C++

The lastest spec changes (#3648, #3422) are implemented by the following
PR:

* open-telemetry/opentelemetry-cpp#2281
* open-telemetry/opentelemetry-cpp#2303

so C++ can be marked compliant with the specification.
@kes2464
Copy link

kes2464 commented Nov 27, 2023

How about :? Prometheus also allows colons if that is a concern.

@jack-berg
Copy link
Member

@kes2464 can you open a new issue to discuss :?

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
None yet
Development

Successfully merging a pull request may close this issue.

10 participants