Related discussion: #8402

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

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.

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?

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

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.

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)

• 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)

👍

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.

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

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.

I will reopen this pr, We can work together.

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

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

@pavelvodrazka Continue in PR

@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 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

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

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.

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

@trask Can you please also take a look?

@trask Can you please also take a look?

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