Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Should use Set() instead of Add() in HTTPHeadersCarrier #191

Merged
merged 1 commit into from
Sep 8, 2018

Conversation

jeremyxu2010
Copy link
Contributor

@jeremyxu2010 jeremyxu2010 commented Aug 22, 2018

Should use Set() instead of Add() in HTTPHeadersCarrier.

Resolves #159

Should use Set() instead of Add() in HTTPHeadersCarrier. see opentracing#159
@yurishkuro
Copy link
Member

I agree that this is probably the right change, but it might break users who already depend on the existing behavior (e.g. by writing the same header multiple times to the carrier).

@tedsuo

@szuecs
Copy link

szuecs commented Sep 7, 2018

@yurishkuro It breaks the contract of the wireformat, so this is IMO a bug fix or am I wrong?

On the other hand the JVM lightstep library parse the data and crashes if there is a list of string in the header. I am interested, how other libraries will handle this case.
We just had a prodcution incident in our CI/CD UI, because of that.
JVM lib from lightstep says:

 WARN [manifold-pool-2-755] c-l-s.tracing - Unable to parse parent span information {"ot-tracer-spanid" "<spanid1>,<spanid2>", "ot-tracer-traceid" "<traceid1>,<traceid2>", "ot-tracer-sampled" "true,true"}.
                                     java.lang.Thread.run                 Thread.java: 748
                                                      ...                                 
                 manifold.executor/thread-factory/reify/f                executor.clj:  44
                 io.aleph.dirigiste.Executor$Worker$1.run               Executor.java:  62
                                                      ...                                 
            aleph.http.server/handle-request/fn/f--auto--                  server.clj: 157
      cdp-log-service.tracing/wrap-parent-span-context/fn                 tracing.clj: 131
            cdp-log-service.tracing/headers->span-context                 tracing.clj: 127
                                                      ...                                 
       com.lightstep.tracer.shared.AbstractTracer.extract         AbstractTracer.java: 362
com.lightstep.tracer.shared.HttpHeadersPropagator.extract  HttpHeadersPropagator.java:   9
com.lightstep.tracer.shared.HttpHeadersPropagator.extract  HttpHeadersPropagator.java:  15
    com.lightstep.tracer.shared.TextMapPropagator.extract      TextMapPropagator.java:  10
    com.lightstep.tracer.shared.TextMapPropagator.extract      TextMapPropagator.java:  42
           com.lightstep.tracer.shared.Util.fromHexString                   Util.java:  57
                              java.math.BigInteger.<init>             BigInteger.java: 479
                               java.lang.Integer.parseInt                Integer.java: 580
           java.lang.NumberFormatException.forInputString  NumberFormatException.java:  65
java.lang.NumberFormatException: For input string: "49de,46"

A colleague wrote
lightstep/lightstep-tracer-go#160, for lightstep, but this seems more a workaround for one product, if you agree that this is a bug.

szuecs added a commit to szuecs/opentracing-go that referenced this pull request Sep 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants