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

Exporting fails if a 204 No Content is returned #4363

Closed
gouthamve opened this issue Jul 25, 2023 · 1 comment · Fixed by #4365
Closed

Exporting fails if a 204 No Content is returned #4363

gouthamve opened this issue Jul 25, 2023 · 1 comment · Fixed by #4365
Labels
bug Something isn't working

Comments

@gouthamve
Copy link
Member

Description

The current http exporters expect a http.StatusOK (200) response code. But this is not required by the spec and also not something the collector requires.

Trying to use the exporter in a test fails with:

failed to upload metrics: failed to send metrics to http://localhost:9090/api/v1/otlp/v1/metrics: 204 No Content
@gouthamve gouthamve added the bug Something isn't working label Jul 25, 2023
gouthamve added a commit to gouthamve/prometheus that referenced this issue Jul 25, 2023
It is what the OTEL Golang SDK expect :(

open-telemetry/opentelemetry-go#4363

Signed-off-by: Goutham <gouthamve@gmail.com>
gouthamve added a commit to gouthamve/prometheus that referenced this issue Jul 25, 2023
It is what the OTEL Golang SDK expect :(

open-telemetry/opentelemetry-go#4363

Signed-off-by: Goutham <gouthamve@gmail.com>
@dmathieu
Copy link
Member

dmathieu commented Jul 26, 2023

There is actually a requirement to return a 200 from full success exports: https://github.com/open-telemetry/opentelemetry-proto/blob/main/docs/specification.md#full-success-1
The spec also mentions than any other unspecified HTTP response should conform to the HTTP specification: https://github.com/open-telemetry/opentelemetry-proto/blob/main/docs/specification.md#all-other-responses

Which I read as: there shouldn't be a 204. But if there's one, we should handle it anyway.

and also not something the collector requires.

This link isn't about returning NoContent. It's about partial successes (which we do handle), and which still must return a 200.
See https://github.com/open-telemetry/opentelemetry-go/blob/main/exporters/otlp/otlpmetric/otlpmetrichttp/client.go#L195

With that said, I agree with you that we should handle NoContent. I've opened a PR for that.

gouthamve added a commit to prometheus/prometheus that referenced this issue Jul 28, 2023
* Add OTLP Ingestion endpoint

We copy files from the otel-collector-contrib. See the README in
`storage/remote/otlptranslator/README.md`.

This supersedes: #11965

Signed-off-by: gouthamve <gouthamve@gmail.com>

* Return a 200 OK

It is what the OTEL Golang SDK expect :(

open-telemetry/opentelemetry-go#4363

Signed-off-by: Goutham <gouthamve@gmail.com>

---------

Signed-off-by: gouthamve <gouthamve@gmail.com>
Signed-off-by: Goutham <gouthamve@gmail.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.

2 participants