search

kamon-io / kamon-logback

12 stars 17 forks source link

Update MDC with traceID and spanID #4

Closed milenkovicm closed 6 years ago

milenkovicm commented 6 years ago

Hi guys,

I'd like to put traceID and spanID in MDC map as some libraries (e.g logstash/logstash-logback-encoder) will encode all data available in MDC without any additional configuration.

milenkovicm commented 6 years ago

Hi @ivantopo could you please have a look if this is something you'd merge to master?

ivantopo commented 6 years ago

Hey @milenkovicm! I'm a bit tied up until end of day, will check it out then :)

milenkovicm commented 6 years ago

Hi @ivantopo I'm not sure that I understand part of the comment about

There is one additional thing that this should have, which is making sure that this behavior can be disabled and other keys could be added to the MDC

can you give some more clarifications, not sure which other keys you're referring to.

milenkovicm commented 6 years ago

Hi @ivantopo, updated according to your comments, please have a look whenever you have time.

ivantopo commented 6 years ago

hey @milenkovicm!

There is one additional thing that this should have, which is making sure that this behavior can be disabled and other keys could be added to the MDC

By that I meant two things:

  1. Copying any information from the Context into the MDC should be something that you can turn on if needed. It takes some CPU time to do so and not everybody needs it. I see you implemented this already!
  2. We could have a setting to decide whether TraceID/SpanID should be copied to the MDC and additionally we could provide a list of key names to be copied to the MDC as well. Many people propagate some sort of additional RequestID or UserID, those would be in the Context and ideally should be copied as well.

Since item 1 is already implemented and the actual reason for this PR I think we can merge it and work on the second item afterwards. What do you think?

milenkovicm commented 6 years ago

Thanks for your comment @ivantopo, My main motivation for this PR was to be able to correlate traces with logs, for that use traceid is sufficient. But I also see value of having other keys there.

Please merge this one, we can add additional keys later

ivantopo commented 6 years ago

Hey @milenkovicm! man, sorry, I forgot to ask you to electronically sign the CLA, please sign it and we will merge this, thanks!

milenkovicm commented 6 years ago

done

ivantopo commented 6 years ago

thanks a lot @milenkovicm! :tada:

milenkovicm commented 6 years ago

thank you! let me know when you release it so i can update deps