-
Notifications
You must be signed in to change notification settings - Fork 931
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)