Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kamon Logback logs empty values for context entries that are not in the current context #918

Closed
yiminyuan opened this issue Dec 15, 2020 · 2 comments
Labels

Comments

@yiminyuan
Copy link

yiminyuan commented Dec 15, 2020

  • Scala 2.12.12
  • Kamon 2.1.9

We currently use a JSON logger. If I add this config: kamon.instrumentation.logback.mdc.copy.entries = [ foo ], then foo would be logged as an empty value even though foo is not in the current context:

{
  "foo" : ""
}

So the workaround is adding the following code to logback.xml and remove kamon.instrumentation.logback.mdc.copy.entries = [ foo ]:

<conversionRule conversionWord="contextEntry" converterClass="kamon.instrumentation.logback.tools.ContextEntryConverter" />

<pattern>
  <omitEmptyFields>true</omitEmptyFields>
  <pattern>
    {
      "foo":"%contextEntry{foo}"
    }
  </pattern>
</pattern>

Temporary fix
In https://github.com/kamon-io/Kamon/blob/master/instrumentation/kamon-logback/src/main/scala/kamon/instrumentation/logback/LogbackInstrumentation.scala, it seems only context tags that are in the current context are reported.

currentContext.tags.iterator().foreach(t => {
  MDC.put(t.key, Tag.unwrapValue(t).toString)
})

We have a very simple use case that all K-V pairs have the string type. So we use context tags instead of context entries.

Expected behavior
If a context entry is not in the current context, then it should not be logged.

Proposed fix
In https://github.com/kamon-io/Kamon/blob/master/instrumentation/kamon-logback/src/main/scala/kamon/instrumentation/logback/LogbackInstrumentation.scala, because of Context.key[Any](key, ""), which means "" is returned by default if key is not in the current context. Hence, we can add extra if-check like this to eliminate the issue:

settings.mdcCopyKeys.foreach { key =>
  currentContext.get(Context.key[Any](key, "")) match {
    case Some(value) =>
      val s = value.toString
      if(s.nonEmpty)
        MDC.put(key, s)
    case keyValue if keyValue != null =>
      val s = keyValue.toString
      if(s.nonEmpty)
        MDC.put(key, s)
    case _ => // Just ignore the nulls.
  }
}
@SimunKaracic
Copy link
Contributor

Fixed in 2.1.13

@yiminyuan
Copy link
Author

Awesome! This commit 85717aa looks good. Thank you for fixing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants