Add synchronous processing and HTTP status code validation to OtlpHttpTraceExporter #1005
Conversation
devjoonn
requested review from
ArielDemarco,
bryce-b,
nachoBonafonte and
vvydier
as code owners
|
@bryce-b @nachoBonafonte @vvydier @ArielDemarco |
| var request = createRequest(body: body, endpoint: endpoint) | ||
| if let headers = envVarHeaders { | ||
| headers.forEach { key, value in | ||
| request.addValue(value, forHTTPHeaderField: key) | ||
| } | ||
| } else if let headers = config.headers { | ||
| headers.forEach { key, value in | ||
| request.addValue(value, forHTTPHeaderField: key) | ||
| } | ||
| } |
There was a problem hiding this comment.
I’ve confirmed that the headers are already being added during the createRequest stage.
I’ll make a commit with the fix. Thank you for the review!
ArielDemarco
left a comment
ArielDemarco
left a comment
There was a problem hiding this comment.
Hey @devjoonn , thanks for the contribution!
The OTLP HTTP Trace Exporter had an issue where it always returned .success regardless of the actual HTTP request result. This prevented callers from detecting failures even when network errors or 4xx/5xx responses occurred.
I don’t think it's necessarily an error to not propagate the HTTP request result synchronously via the return value.
According to the official spec, what export is required to signal is that the export task (whether synchronous or asynchronous) has completed and not necessarily the outcome of the underlying network request. Quoting the spec directly:
Depending on the implementation the result of the export may be returned to the Processor not in the return value of the call to Export() but in a language specific way for signaling completion of an asynchronous task
Additionally, whether an export ultimately succeeds or fails (and how retries, does exponential backoff, or concurrent requests are handled) is explicitly the responsibility of the exporter, not the consumer (aka. the processor), as described in the same section of the spec.
All that being said, I'm not opposed to this change. In fact, waiting for the async task to complete can be useful to better reflect task completion semantics. However, per the spec, it's important to ensure that this does not block indefinitely. In particular, I think there should be a timeout when waiting on the semaphore to avoid stale exporters. Quoting again the spec:
Export() MUST NOT block indefinitely, there MUST be a reasonable upper limit after which the call must time out with an error result (Failure}
Finally, I'm seeing several test failures, which I assume are related to the behavioral change introduced here
2 participants
The OTLP HTTP Trace Exporter had an issue where it always returned .success regardless of the actual HTTP request result. This prevented callers from detecting failures even when network errors or 4xx/5xx responses occurred.
To fix this, the exporter now waits for the HTTP request to finish and returns the correct result based on the actual response. Additionally, timeout handling has been added to prevent indefinite blocking, ensuring compliance with the OpenTelemetry spec requirement that "Export() MUST NOT block indefinitely, there MUST be a reasonable upper limit after which the call must time out with an error result (Failure)".
The timeout implementation follows the same pattern as
OtlpHttpLogExporterfor consistency, usingmin(explicitTimeout ?? infinity, config.timeout)for bothrequest.timeoutIntervalandsemaphore.wait(). The default timeout is 10 seconds, with optional override via theexplicitTimeoutparameter.With this change, the exporter properly returns .failure for network issues or non-2xx responses, and failed spans are safely re-added for retry.