-
Notifications
You must be signed in to change notification settings - Fork 933
Description
opentelemetry-specification/specification/logs/sdk.md
Lines 197 to 199 in 6e57784
| returns `false`. If `disabled` is `false`, `Enabled` returns `true`. It is not | |
| necessary for implementations to ensure that changes to `disabled` are | |
| immediately visible to callers of `Enabled`. |
Why? This should be visible at the same time as for callers of Emit a LogRecord. Otherwise, the result Enabled is not trustworthy.
One may see this as acceptable:
It is not necessary for implementations to ensure that changes to any of these
parameters are immediately visible to callers of `Enabled` and `Emit a LogRecord`.
However, I do not think it is a good idea. If users changes a configuration, they expect that it is reflected in subsequent method calls.
I think that such statement can promote buggy, not properly synchronized implementations.
It would be also not consistent with the rationale behind Enabled:
opentelemetry-specification/specification/logs/api.md
Lines 153 to 156 in 6e57784
| The returned value is not always static, it can change over time. The API | |
| SHOULD be documented that instrumentation authors needs to call this API each | |
| time they [emit a LogRecord](#emit-a-logrecord) to ensure they have the most | |
| up-to-date response. |
It is also not consistent with the Enabled section in this (SDK) document:
opentelemetry-specification/specification/logs/sdk.md
Lines 208 to 212 in 6e57784
| `Enabled` MUST return `false` when either: | |
| - there are no registered [`LogRecordProcessors`](#logrecordprocessor), | |
| - **Status**: [Development](../document-status.md) - `Logger` is disabled | |
| ([`LoggerConfig.disabled`](#loggerconfig) is `true`), |
I propose remove this statement completely and let the SIGs decide how they want to synchronize.
This statement is also in other parts of the specification (TracerConfig and MeterConfig) and should be removed from those places as well. Therefore, this comment should probably be better addressed in a separate PR.
Originally posted by @pellared in #4612 (comment)