Clarify logs export concurrency #4173
Conversation
pellared
added
spec:logs
From https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md#export: > `Export` will never be called concurrently for the same exporter instance. From our SDK perspective this change will make https://pkg.go.dev/go.opentelemetry.io/otel/exporters/stdout/stdoutlog concurrent-safe when used with the simple processor. Before the change, there can be multiple goroutines calling `Write` in parallel (via `json.Encoder.Encode`). ### Remarks [Maybe we should simply state that "whole" exporter implementation does not need to be concurrent-safe?](open-telemetry/opentelemetry-specification#4173 (comment)) However: 1. we would need to make complex changes in `BatchExporter` to synchronize the `Export`, `Shutdown`, `ForceFlush` calls 2. we would need to update all exporters (remove synchronization) and simple exporter (add locks to `Shutdown`, `ForceFlush`) 3. I am 100% not sure if this would be compliant with the specification - I think it would be complaint because we would simply give stronger safety-measures We should probably discuss it separately, but I wanted to highlight my though process. Even if we decide that simple and batch processors to synchronize all calls then I would prefer to address it in a separate PR. Related spec clarification PR: - open-telemetry/opentelemetry-specification#4173
While this is definitely feasible (and was done before too), I think its best to model them as exporters itself. Anyway, that should not block this PR, which is clarifying existing wording. |
Co-authored-by: Cijo Thomas <[email protected]>
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
There was a problem hiding this comment.
I think this PR fixes an issue in the spec. We can tell callers that its invalid to call export concurrently, but it's not enforceable. We CAN add normative language saying that built-in processors must never call export concurrently, which is what this PR does.
Separately, there have been requests to add support for concurrent export (e.g. to increase export throughput). For this to work, we likely need to evolve the export specification, allowing exports to opt-in to concurrent calls to export via some new operation like boolean supportsConcurrentExport(). Once exporters have this, we can evolve the processor specs to be able to support calling export concurrently IF the exporter supports it.
But those seems like future steps which this PR doesn't prevent.
Co-authored-by: Reiley Yang <[email protected]>
Same as open-telemetry#4173 for metrics SDK. Towards open-telemetry#4134
Same as open-telemetry#4173 for trace SDK. Fixes open-telemetry#4134
Towards #4134 (the same should be done for trace and metrics signal)
Inspired by #4163
Related PR in Go SDK: open-telemetry/opentelemetry-go#5666
Changes
Each implementation MUST document the concurrency characteristics the SDK requires of the exporter.to a better placeCurrently the spec says only "Export will never be called concurrently for the same exporter instance"
Remarks
For exporting to OS native tracing systems like ETW (Windows), user_events (Linux) we may provide dedicated
LogRecordProcessorimplementations which emit directly to them (without even implementingLogRecordExporter).This is a change in
Stableportion of specification which does not include normative language. Therefore, it may be considered as a breaking change or a as a bugfix (clarification) of the specification.