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

OTel Instrument names not conforming to otel spec #8346

Closed
codeboten opened this issue Aug 31, 2023 · 7 comments · Fixed by #8575
Closed

OTel Instrument names not conforming to otel spec #8346

codeboten opened this issue Aug 31, 2023 · 7 comments · Fixed by #8575
Assignees
Labels
bug Something isn't working

Comments

@codeboten
Copy link
Contributor

The OpenTelemetry instruments created for the Collector's internal metrics use a / separator. This breaks when trying to upgrade to the latest version of the otel go SDK as per the change: open-telemetry/opentelemetry-go#4210

@codeboten codeboten added the bug Something isn't working label Aug 31, 2023
@codeboten codeboten self-assigned this Aug 31, 2023
@codeboten
Copy link
Contributor Author

This may be addressed when open-telemetry/opentelemetry-specification#3422 is closed

@dashpole
Copy link
Contributor

dashpole commented Sep 5, 2023

It is possible (but not pretty) to work around this with Views, which do not do any validation on the stream name.

@dashpole
Copy link
Contributor

dashpole commented Sep 7, 2023

Also, the error about "/" can be ignored. You still get a valid instrument. cc @MrAlias

@codeboten
Copy link
Contributor Author

codeboten commented Sep 7, 2023

@dashpole are you saying the result is a valid instrument even with the invalid instrument name error?

error creating batch processor telemetry: invalid instrument name: processor/batch/batch_size_trigger_send: must only contain [A-Za-z0-9_.-]

@codeboten
Copy link
Contributor Author

Ah yes i see in the code that the instrument is returned regardless of what the validation returns. Is it safe to ignore this error then?

@MrAlias
Copy link
Contributor

MrAlias commented Sep 7, 2023

Ah yes i see in the code that the instrument is returned regardless of what the validation returns. Is it safe to ignore this error then?

It should be safe in the sense that the SDK will not have a problem with this (it should process the instrument measurements as before). The only issue would be downstream. If the data goes to a place that expects the instrument names to conform to the OTel spec, that expectation will not be meet.

codeboten pushed a commit that referenced this issue Sep 8, 2023
Updating dependencies. Includes a workaround #8346, which causes the
collector to ignore instrument name errors returned from instantiating
instruments in otel.

---------

Signed-off-by: Alex Boten <aboten@lightstep.com>
@codeboten
Copy link
Contributor Author

Once open-telemetry/opentelemetry-go#4500 is released, we should be able to revert the workaround to ignore the error

codeboten pushed a commit to codeboten/opentelemetry-collector that referenced this issue Oct 3, 2023
This fixes open-telemetry#8346

Signed-off-by: Alex Boten <aboten@lightstep.com>
codeboten pushed a commit to codeboten/opentelemetry-collector that referenced this issue Oct 4, 2023
This fixes open-telemetry#8346

Signed-off-by: Alex Boten <aboten@lightstep.com>
codeboten pushed a commit that referenced this issue Oct 4, 2023
The workaround had been put in place because of a restriction on `/`
characters in the name of the instrument. This is no longer an issue.
This PR fixes
#8346

Follows
#8574

---------

Signed-off-by: Alex Boten <aboten@lightstep.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants