Can we mark logging instrumentation as stable when the logs sdk is stable in 1.27.0?

[zeitlinger]

From the SIG meeting 2023-06-01:

The following aspects have to be considered:

Semantic Conventions

All semantic conventions are marked as experimental - but this should not hold us back from making the appenders stable.

Experimental features

An overview which features have semantic conventions.

Feature	Semantic conventions
captureExperimentalAttributes	yes, thread.name, thread.id
captureCodeAttributes	yes, code.filepath, code.namespace, code.function, code.lineno
captureMarkerAttribute	no
captureKeyValuePairAttributes	no - does it make sense? add a prefix?
captureMdcAttributes	no - does it make sense? add a prefix?

Logback

• captureExperimentalAttributes - captures thread name and ID - is already marked as experimental
• captureCodeAttributes - captures code related attributes - is there any reason this should be experimental? Maybe because code attributes are relatively new?
• captureMarkerAttribute - is there any reason this should be experimental?
• captureKeyValuePairAttributes - captures key value pairs from logging events that the user added - looks good to stay as is
• captureMdcAttributes - captures MDC attributes (user supplied list) - any reason this should be marked experimental?

Log4j

• captureExperimentalAttributes - same as logback
• captureMapMessageAttributes - same as captureKeyValuePairAttributes from logback
• captureMarkerAttribute - looks the same as logback,
• captureContextDataAttributes - same as captureMdcAttributes from logback
• captureCodeAttributes is missing here - should it be added?

[jack-berg]

The log4j appender has a static inner builder class that is reflectively called when interpreting the xml configuration. My idea was maybe this builder could be moved to an internal package such that we can maintain the ability to change the setters.

[laurit]

• captureMarkerAttribute - code markers seem to be deprecated - should this be removed before stabilization?

The @Deprecated there is a typo, should be @SuppressWarnings("deprecation"). ILoggingEvent.getMarker() was deprecated in favour of ILoggingEvent.getMarkerList in logback 1.3.0

The log4j appender has a static inner builder class that is reflectively called when interpreting the xml configuration. My idea was maybe this builder could be moved to an internal package such that we can maintain the ability to change the setters.

Considering that from the xml configuration it is impossible to tell that these setters are in an internal class users won't be able to tell that these properties are experimental.

[zeitlinger]

Considering that from the xml configuration it is impossible to tell that these setters are in an internal class users won't be able to tell that these properties are experimental.

I agree. An internal class can only prevent that users use the appender programmatically, e.g. by extending it - which we could prevent by making it final.

Would it be enough to add "experimental" in the feature names, as in "captureExperimentalAttributes"?
Which features would we consider experimental?

[laurit]

Would it be enough to add "experimental" in the feature names, as in "captureExperimentalAttributes"? Which features would we consider experimental?

It depends on what a non-experimental feature that is disabled by default means. If we can agree that disabled by default features can implicitly be treated as experimental then we could skip adding the Experimental to their names. If having the feature disabled by default isn't enough to make it experimental then https://opentelemetry.io/docs/specs/otel/telemetry-stability/#stable-instrumentations

Stable instrumentations authored by OpenTelemetry SHOULD NOT produce telemetry that is not described by OpenTelemetry semantic conventions.

forces us to make all features experimental that do not have semantic conventions.

[zeitlinger]

Would it be enough to add "experimental" in the feature names, as in "captureExperimentalAttributes"? Which features would we consider experimental?

It depends on what a non-experimental feature that is disabled by default means. If we can agree that disabled by default features can implicitly be treated as experimental then we could skip adding the Experimental to their names. If having the feature disabled by default isn't enough to make it experimental then https://opentelemetry.io/docs/specs/otel/telemetry-stability/#stable-instrumentations

I'd say adding an explicit "experimental" is more in the spirit of https://opentelemetry.io/docs/specs/otel/telemetry-stability/#stable-instrumentations

Stable instrumentations authored by OpenTelemetry SHOULD NOT produce telemetry that is not described by OpenTelemetry semantic conventions.

forces us to make all features experimental that do not have semantic conventions.

I tend to agree with that.
I added a table in the description listing which features have semantic conventions.

[trask]

• captureExperimentalAttributes - captures thread name and ID - is already marked as experimental

👍

• captureCodeAttributes - captures code related attributes - is there any reason this should be experimental? Maybe because code attributes are relatively new?

we'd need to push to get a semantic conventions working group established for marking these stable

• captureMarkerAttribute - is there any reason this should be experimental?

this is using logback.marker attribute key, we should discuss whether there's a better / more generic approach that would work across other logging library instrumentation

• captureKeyValuePairAttributes - captures key value pairs from logging events that the user added - looks good to stay as is

👍

• captureMdcAttributes - captures MDC attributes (user supplied list) - any reason this should be marked experimental?

this is prefixing the attribute keys with logback.mdc.*, we should discuss whether there's a better / more generic approach that would work across other logging library instrumentation

[zeitlinger]

• captureCodeAttributes - captures code related attributes - is there any reason this should be experimental? Maybe because code attributes are relatively new?

we'd need to push to get a semantic conventions working group established for marking these stable

I'd suggest we do that as a next step and mark code attributes as experimental for now.

this is using logback.marker attribute key, we should discuss whether there's a better / more generic approach that would work across other logging library instrumentation AND
this is prefixing the attribute keys with logback.mdc.*, we should discuss whether there's a better / more generic approach that would work across other logging library instrumentation

we could simply remove the logback. prefix and then it would be applicable to other logging libraries

[trask]

this is using logback.marker attribute key, we should discuss whether there's a better / more generic approach that would work across other logging library instrumentation AND
this is prefixing the attribute keys with logback.mdc.*, we should discuss whether there's a better / more generic approach that would work across other logging library instrumentation

we could simply remove the logback. prefix and then it would be applicable to other logging libraries

Log4j uses the term Context Data instead of MDC, and so currently the log4j instrumentation uses log4j.context_data.* instead of log4j.mdc.*.

I could potentially see removing the full prefix and just adding MDC / Context Data keys directly as log attributes.

[trask]

before marking appenders stable, we should discuss/align on their dependence on GlobalOpenTelemetry, see #8688 (comment)

[trask]

Another logging appender stability blocker: open-telemetry/opentelemetry-specification#3518

[trask]

All semantic conventions are marked as experimental - but this should not hold us back from making the appenders stable.

I think exception semantic conventions on logs not being stable may be a blocker: https://github.com/open-telemetry/semantic-conventions/blob/main/specification/logs/semantic_conventions/exceptions.md

[trask]

this is a good issue to follow/contribute to: open-telemetry/semantic-conventions#134

[trask]

Would be good to implement #8354 (comment)