opentelemetry-specification icon indicating copy to clipboard operation
opentelemetry-specification copied to clipboard

Refine only logs reads are permitted section

Open pellared opened this issue 1 year ago • 2 comments
trafficstars

This is a portion added by @jack-berg in https://github.com/open-telemetry/opentelemetry-specification/pull/2969. I would rather not add it may lead to implementing processors which may lead to race conditions.

I could change it to something like

processing) only reads should be used
as modifications can lead to race conditions.

We could also say that modifications could be done if the log record used for asynchronous processing is cloned (e.g. via an isolating processor introduced here https://github.com/open-telemetry/opentelemetry-specification/pull/4062). But I think such clarifications should be better scoped in a separate PR.

@cijothomas, @jack-berg, thoughts? I am fine applying my suggestion if @jack-berg finds it good, but I would prefer not defining anything about cloning+modifications for asynchronous processing in scope of this PR.

Originally posted by @pellared in https://github.com/open-telemetry/opentelemetry-specification/pull/4067#discussion_r1656585974

pellared avatar Jul 11 '24 17:07 pellared

@pellared The PR linked in this issue has materially changed. Is this issue still applicable?

tigrannajaryan avatar Aug 21 '24 15:08 tigrannajaryan

@open-telemetry/technical-committee, yes it is still applicable.

What is more concurrent reads can also be unsafe if the log record was synchronously modified in a processor. That is why in Go Logs SDK we are cloning the log record before enqueuing it in the batch processor. See: https://github.com/open-telemetry/opentelemetry-go/blob/48fedfa58e35b5f3b08fe54e45cfad441dd82426/sdk/log/batch.go#L185-L187.

I can be a sponsor and try to refine the spec.

pellared avatar Sep 04 '24 10:09 pellared