Allow configuring MDC key names for trace_id, span_id, trace_flags

[philsttr]

Is your feature request related to a problem? Please describe.

The MDC keys used for trace_id, span_id, and trace_flags are currently hardcoded in LoggingContextConstants.

Sometimes applications have different standards for MDC key names. Such as using abbreviated names, camelCase, or a prefix.

For example, using abbreviated names or camelCase can save logging costs when billions of log messages are sent to a cloud logging provider that bills by data volume.

Describe the solution you'd like

I'd like the ability to configure the MDC keys used for the trace_id, span_id, and trace_flags fields.

As a stretch goal, I would also like the ability to turn off the trace_flags MDC key.

Perhaps something like a property containing key value pairs, with keys being the current names, and values being the new name (or empty for exclusion).

For example, to rename trace_id and span_id, and exclude trace_flags

otel.instrumentation.logback-mdc.keys=trace_id=trace,span_id=span,trace_flags=

(or equivalent for the log4j-mdc, logback-mdc, and log4j-context-data instrumentations)

Describe alternatives you've considered

Some log encoders (such as those provided by logstash-logback-encoder) allow renaming or excluding MDC keys.

But many log encoders do not have this feature.

Additional context

No response

[mateuszrzeszutek]

Related discussion: #8402

[trask]

hey @philsttr,

can you give a bit of context around why you can't rename them (and remove trace flags) in the encoder? thx

e.g.

    <encoder>
      <pattern>%d{HH:mm:ss.SSS} tid=%X{trace_id} sid=%X{span_id} %msg%n</pattern>
    </encoder>

https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/logback/logback-mdc-1.0/library#usage

[philsttr]

A couple possibilities:

1. Say I am using logback's pattern layout, and I want all MDC keys to be output in the logs. I don't want to enumerate all possible MDC keys in the pattern, to give the application flexibility to use any keys needed. Therefore I configure the pattern with %mdc without a key qualifier, as supported by logback.
2. Say I am not using logback's pattern layout, but I'm using some other encoder that includes all MDC keys.

[ty-elastic]

Hi, as another example, say I want to use Elasticsearch as a backend for app log data. In this scenario, it would be common for me to employ the Elastic ECS Encoder to format my log statements. The Elastic ECS Encoder will dump all MDC args as is to the JSON log line; there is no configuration option to rename certain MDC arguments.

The Elasticsearch backend wants "trace_id" to be "trace.id" and "span_id" to be "span.id" to connect the dots from the log lines to the trace data, per ECS conventions. To make this work today, I currently need to rename these fields on ingest. Ideally, we could rename them at the source to follow ECS conventions... Given the existing choice of naming, a configurable option probably makes sense. thoughts?

[trask]

it seems like there's enough interest in this feature that we'd accept it if someone wishes to contribute it. the most complicated part will probably be agreeing on configuration property names

two concerns with this proposal:

otel.instrumentation.logback-mdc.keys=trace_id=trace,span_id=span,trace_flags=

• it would be nice to avoid squashing everything into a single key, this will be especially nice once we support yaml configuration
• it might be nice to have common properties instead of logging framework specific properties

[SylvainJuge]

Hi !
@trask by "not squashing everything in a single key", do you mean having a dedicated key per attribute ?
+1 on avoiding framework specific properties, some do not even have an MDC like java.util.logging, in which case the agent could provide a fallback and inject the key/value attributes in the message itself or better use a structured log format.

Here I would suggest to have the following configuration options:

• otel.instrumentation.logging.keys.trace_id = trace_id (default)
• otel.instrumentation.logging.keys.span_id = span_id (default)
• otel.instrumentation.logging.keys.trace_flags = trace_flags (default)

When one of these keys is empty, it means the field should not be injected in the MDC.

[philsttr]

Since this is common to multiple instrumentations, and I see the otel.instrumentation.common prefix being used for other property names that are common to multiple instrumentations, perhaps these properties should be named:

• otel.instrumentation.common.logging.keys.trace_id = trace_id (default)
• otel.instrumentation.common.logging.keys.span_id = span_id (default)
• otel.instrumentation.common.logging.keys.trace_flags = trace_flags (default)

[trask]

• otel.instrumentation.common.logging.keys.trace_id = trace_id (default)
• otel.instrumentation.common.logging.keys.span_id = span_id (default)
• otel.instrumentation.common.logging.keys.trace_flags = trace_flags (default)

👍

[pavelvodrazka]

Hi @trask,
it's been a while since the last response, and the issue is still open without any assignee. I see a pull request from @AlchemyDing, but it was closed half a year ago without any explanation. Is there any plan to resolve this request? Is there any way I can help? I am facing the same problem that Phil described in the description.

[trask]

hi @pavelvodrazka! if you'd like to send a PR that would be great

[pavelvodrazka]

Hi @trask,
I sure can. But in the meantime, I tried to build the agent from the original PR #9901 and it works fine in my tests. So it makes more sense to me to use this PR instead creating a new one with the same changes.

[AlchemyDing]

I will reopen this pr, We can work together.

[pavelvodrazka]

Hi @AlchemyDing,
thanks! BTW what was the reason you closed it on the same day you created it?

[AlchemyDing]

Sorry, I suddenly realized that the original branch was deleted by me, so PR cannot be reopened. I will restore it tomorrow. Do you think it's okay？
@pavelvodrazka

[AlchemyDing]

@pavelvodrazka Continue in PR

[pavelvodrazka]

@AlchemyDing Is there anything missing in your PR? I ported the changes from your old PR yesterday and my instrumented app with Logback and Logstash encoder worked fine. I was able to change the MDC keys without any trouble.

I only see failed checks:

1. The muzzle / muzzle (:instrumentation:muzzle3) with error:

   Could not resolve org.apache.camel:camel-core:4.6.0
   

   Which seems like just a wrong dependency. Easy to fix. But that shouldn't be related to your PR, because you haven't changed project dependencies.

2. The required-status-check, but here I don't know what the cause is. This error description is not very explanatory...

   Run exit 1
     exit 1
     shell: /usr/bin/bash -e {0}
   Error: Process completed with exit code 1.
   

[AlchemyDing]

@AlchemyDing Is there anything missing in your PR? I ported the changes from your old PR yesterday and my instrumented app with Logback and Logstash encoder worked fine. I was able to change the MDC keys without any trouble.

I only see failed checks:

1. The muzzle / muzzle (:instrumentation:muzzle3) with error:

   Could not resolve org.apache.camel:camel-core:4.6.0
   

   Which seems like just a wrong dependency. Easy to fix. But that shouldn't be related to your PR, because you haven't changed project dependencies.

2. The required-status-check, but here I don't know what the cause is. This error description is not very explanatory...

   Run exit 1
     exit 1
     shell: /usr/bin/bash -e {0}
   Error: Process completed with exit code 1.
   

The failure of muzzle3 check has nothing to do with this PR, the required status check failure is due to the failure of muzzle3

[pavelvodrazka]

So am I correct that nothing needs to be done with this PR at this time other than a review by the OTel team?

[AlchemyDing]

So am I correct that nothing needs to be done with this PR at this time other than a review by the OTel team?

@pavelvodrazka If you are interested, you can provide suggestions for the code.

[pavelvodrazka]

I took a look yesterday and i looked good to me.

@trask Can you please also take a look?

[trask]

@trask Can you please also take a look?

it looks like the main open issue on the PR is that it needs tests