Proposal: Add ListValue and NoneValue to TraceValue ADT #1248
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I propose adding two new types to the
TraceValueADT, which currently consists ofnatchez/modules/core/shared/src/main/scala/TraceValue.scala
Lines 7 to 15 in fcc2c92
These new types would support two use cases that would be a helpful bridge to otel4s.
ListValueListValuecontains a list ofTraceValues and can be used by backends that have first-class support for list/arrays, such as OpenTelemetry and X-Ray.My immediate use for this is to support adding the
aws.xray.annotationsattribute list, which X-Ray uses to signal that fields should be indexed and not simply recorded. Indexing makes them searchable, which helps tremendously in trace discoverability. Importantly, it does not work to serialize the list as a stringified JSON array. The backend must seeaws.xray.annotations: Slice(["foo","bar","baz"])and not e.g.aws.xray.annotations: Str(["foo", "bar", "baz"]).Other backends can serialize these values to a stringified JSON array or just
mkString(", ").NoneValueAdding
NoneValueallows generic tracing instrumentation to more easily add optional values to spans using a new[A: TraceableValue]: TraceableValue[Option[A]]instance that usesTraceableValue[A]onSomeandNoneValueonNone.This is helpful when semi-autogenerating instrumentation for an final-tagless encoded trait using cats-tagless
Aspect[Foo, TraceableValue, TraceableValue], because the compiler requires an instance ofTraceableValuefor all the domain and codomain types on the trait. e.g.would require
TraceableValue[Option[Int]]andTraceableValue[Option[String]]in scope. We currently handle this by emittingStringValue("None"), but it would be nice to have the option to omit these values entirely.Issues
Does this require a major version bump? MiMa doesn't report any issues, but adding new cases to the ADT could result in
MatchErrors if any downstream code is pattern matching onTraceValue. I thought I remembered reading somewhere that was discouraged, but it's not forbidden by the types, so maybe we should accept that people might be doing it. This might not be worth doing if it requires a major version bump to Natchez. (I know I would prefer to spend time migrating to otel4s as opposed to managing a major upgrade here.)NoneValuehas a suitable alternative today, but I'm not sure it's possible to do whatListValuewould enable without it.