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

Fix bug in PassThroughPropagator #551

Closed

Conversation

NthPortal
Copy link
Contributor

Fix incorrect/invalid optimisation in PassThroughPropagator in which extracting values and then extracting nothing still left the values that were extracted the first time.

Fix incorrect/invalid optimisation in `PassThroughPropagator` in
which extracting values and then extracting nothing still left the
values that were extracted the first time.
@NthPortal NthPortal added the bug Something isn't working label Mar 15, 2024
@NthPortal NthPortal requested a review from iRevive March 15, 2024 16:14
.flatMap(k => getter.get(carrier, k).map(k -> _))
.toList
if (list.isEmpty) ctx else ctx.updated(entriesKey, list)
ctx.updated(entriesKey, entries)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, OpenTelemetry Java keeps original context when no fields were extracted.

@@ -36,4 +36,11 @@ class PassThroughPropagatorSuite extends FunSuite {
val injected = propagator.inject(extracted, Map.empty[String, String])
assert(injected.isEmpty)
}

test("does not inject fields after extracting none") {
Copy link
Contributor

@iRevive iRevive Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to replicate this test with oteljava PassThroughPropagator (the test fails):

test("does not inject fields after extracting none") {
  import scala.jdk.CollectionConverters._
  
  val getter = new TextMapGetter[Map[String, String]] {
    def keys(carrier: Map[String, String]): java.lang.Iterable[String] = carrier.keys.toList.asJava
    def get(carrier: Map[String, String], key: String): String = carrier.get(key).orNull
  }
  
  val setter = new TextMapSetter[scala.collection.mutable.Map[String, String]] {
    def set(carrier: scala.collection.mutable.Map[String, String], key: String, value: String): Unit =
      carrier.update(key, value)
  }
  
  val propagator = PassThroughPropagator.create("foo", "bar")
  val extracted1 = propagator.extract(Context.root(), Map("foo" -> "0"), getter)
  val extracted2 = propagator.extract(extracted1, Map.empty[String, String], getter)
  
  println(extracted1)
  println(extracted2)
  
  val map = scala.collection.mutable.Map.empty[String, String]
  propagator.inject(extracted2, map, setter)
  
  println(map)
  assert(map.isEmpty)
}

The output:

{passthroughpropagator-keyvalues=[foo, 0]}
{passthroughpropagator-keyvalues=[foo, 0]}
HashMap(foo -> 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that feels very wrong, I don't like that at all. the issue is, if you do

  val extracted2 = propagator.extract(extracted1, Map("bar" -> "1"), getter)

instead, it will remove "foo" -> "0" from the context, which seems inconsistent

Copy link
Contributor

@iRevive iRevive Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My expectation from the PassThroughPropagator is that it would try to extract all fields. Let's say the propagator is PassThroughPropagator("foo", "bar"), and I expect both foo and bar to be extracted from the carrier.

And a few scenarios and behaviors that I expect:

  1. carrier = Map("foo" -> "1", "bar" -> "2") - the upstream propagated two headers -> List("foo" -> "1", "bar" -> "2") should be stored in the context
  2. carrier = Map("foo" -> "1") - the upstream propagated only one header -> List("foo" -> "1") should be stored in the context
  3. carrier = Map() - the upstream doesn't propagate any header -> nothing should be stored in the context

The previously stored fields should be overwritten in the context.


In some sense, it would be easier if the PassThroughtPropagator stored values as List[String -> Option[String]].
Then, the behavior would be more intuitive.

For carrier = Map("foo" -> "1") it would be List("foo" -> Some("1"), "bar" -> None).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after chatting with @rossabaker and looking at the spec I think I agree with you, though the spec is not worded super clearly

@NthPortal NthPortal closed this Mar 22, 2024
@NthPortal NthPortal deleted the PassThroughPropagator-fix branch March 22, 2024 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants