Skip to content

feat!: add ContextPair to StructuredLogger #903

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jhenahan
Copy link

@jhenahan jhenahan commented Mar 22, 2025

Motivation

addContext has one sharp edge on it that seems to come up a lot during onboarding: if you want to add a single context element, the syntax is unintuitive.

Say we want to add the context "foo" -> "bar" to a logger. Naturally, one might reach for addContext(pairs: (String, Shown)*), but alas, after erasure we have

logger.addContext(
  "foo" -> "bar" // Tuple2
)

which doesn't conform to either of Map or Seq. This leaves us with these options:

logger.addContext(
  Map(
    "foo" -> bar.show
  )
)

logger.addContext(
  "foo" -> (bar: Shown)
)

Neither feels great, and it would be really pleasant to have consistent syntax for single-element and multi-element context data without having to use Map literals and explicit .show/.toString.

Result

This patch hijacks StructuredLogger's implicit scope to do a bit of juggling and resolve the erasure issue, and in the process cleans up a chunk of duplicative code in its descendants.

I've tested this in the work code, and it appears to work as expected for any subtype of StructuredLogger. The signature ain't exactly beautiful, but the syntactic consistency is gratifying.

I'll add tests if this looks tolerable, just wanted to get the idea out of my head.

Motivation
==========

`addContext` has one sharp edge on it that seems to come up a lot during
onboarding: if you want to add a single context element, the syntax is
unintuitive.

Say we want to add the context `"foo" -> "bar"` to a logger. Naturally,
one might reach for `addContext(pairs: (String, Shown)*)`, but alas,
after erasure we have

```scala
logger.addContext(
  "foo" -> "bar" // Tuple2
)
```

which doesn't conform to either of `Map` or `Seq`. This leaves us with
these options:

```scala
logger.addContext(
  Map(
    "foo" -> bar.show
  )
)

logger.addContext(
  "foo" -> (bar: Shown)
)
```

Neither feels _great_, and it would be really pleasant to have
consistent syntax for single-element and multi-element context data
without having to use `Map` literals and explicit `.show`/`.toString`.

Result
======

This patch hijacks `StructuredLogger`'s implicit scope to do a bit of
juggling and resolve the erasure issue, and in the process cleans up a
chunk of duplicative code in its descendants.

I've tested this in the work code, and it appears to work as expected
for any subtype of `StructuredLogger`. The signature ain't exactly
beautiful, but the syntactic consistency is gratifying.
@jhenahan jhenahan force-pushed the feat/context-pair branch from b0afda5 to 45232f2 Compare March 22, 2025 03:51
@rossabaker
Copy link
Member

Thanks!

This breaks binary compatibility. It might be possible to regain it by making the old signature private.

I have never much liked cats.Show. It should only be used for debugging, never for general serialization. I would not be opposed to just deprecating the method. But log4cats is arguably all about debugging, so maybe my complaint doesn't apply here. If people like it, and if compatibility can be preserved, I think the basic approach here is reasonable.

@jhenahan
Copy link
Author

I had noticed that. I'll see if I can recover binary compatibility as you say. I have a few other ideas around structured logging, mainly around avoiding all the Map appending, though a Show alternative has also occurred to me, maybe with some derivation via Show when possible to avoid too much rework in existing code bases. There are some interesting ideas in scribe that might be adaptable to log4cats, or maybe a revived direct integration of the two libraries.

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

Successfully merging this pull request may close these issues.

2 participants