Skip to content

[exporters/otlptracehttp] Do not log errors for 2XX response#3707

Closed
songy23 wants to merge 4 commits into
open-telemetry:mainfrom
songy23:otlphttp-fix
Closed

[exporters/otlptracehttp] Do not log errors for 2XX response#3707
songy23 wants to merge 4 commits into
open-telemetry:mainfrom
songy23:otlphttp-fix

Conversation

@songy23

songy23 commented Feb 10, 2023

Copy link
Copy Markdown
Member Author

cc @dineshg13

@codecov

codecov Bot commented Feb 10, 2023

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.7%. Comparing base (f5a1497) to head (c45623a).
Report is 1333 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3707   +/-   ##
=====================================
  Coverage   79.7%   79.7%           
=====================================
  Files        171     171           
  Lines      12673   12673           
=====================================
  Hits       10103   10103           
  Misses      2357    2357           
  Partials     213     213           
Files with missing lines Coverage Δ
exporters/otlp/otlptrace/otlptracehttp/client.go 77.5% <100.0%> (ø)

... and 2 files with indirect coverage changes

Comment thread exporters/otlp/otlptrace/otlptracehttp/client.go Outdated
@songy23 songy23 changed the title [exporters/otlptracehttp] Do not log errors for 202 Accepted response [exporters/otlptracehttp] Do not log errors for 2XX response Feb 10, 2023

@Aneurysm9 Aneurysm9 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hoisting this as it was on a comment chain that got resolved:

On success, the server MUST respond with HTTP 200 OK.

If the request is only partially accepted (i.e. when the server accepts only parts of the data and rejects the rest), the server MUST respond with HTTP 200 OK.

Nothing other than a 200 OK response is according to spec.

@dineshg13

dineshg13 commented Feb 10, 2023

Copy link
Copy Markdown
Member

Hoisting this as it was on a comment chain that got resolved:

On success, the server MUST respond with HTTP 200 OK.

If the request is only partially accepted (i.e. when the server accepts only parts of the data and rejects the rest), the server MUST respond with HTTP 200 OK.

Nothing other than a 200 OK response is according to spec.

The spec says All other HTTP responses that are not explicitly listed in this document should be treated according to HTTP specification. . Any 2xx should be treated as non-error . https://umbraco.com/knowledge-base/http-status-codes/#2xx

cc @Aneurysm9

@songy23

songy23 commented Feb 10, 2023

Copy link
Copy Markdown
Member Author

@Aneurysm9

Copy link
Copy Markdown
Member

The 2xx (Successful) class of status code indicates that the client's request was successfully received, understood, and accepted.

The OTel spec for OTLP/HTTP says that servers must respond to fully or partially successful requests with 200 OK. That covers all successful HTTP response scenarios. Any server that responds with 201 Created or 202 Accepted are non-conformant. Encountering a non-conformant server is an error condition.

That other SDKs do something different doesn't mean that we need to follow their lead.

@dineshg13

Copy link
Copy Markdown
Member

@Aneurysm9 @songy23 I have created an issue on Spec . open-telemetry/opentelemetry-specification#3203 . It looks like we need a clarification on the spec for this issue.

@Aneurysm9 Aneurysm9 added the blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made label Feb 13, 2023
@github-actions

Copy link
Copy Markdown
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions Bot added the Stale label Feb 13, 2025
@pellared

pellared commented Feb 13, 2025

Copy link
Copy Markdown
Member

This is already addressed

@pellared pellared closed this Feb 13, 2025
@songy23
songy23 deleted the otlphttp-fix branch February 13, 2025 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[exporters/otlptracehttp] Got error messages despite export succeeded

5 participants