feat: split extract_context, fix tracestate parsing on empty string, adjust to spec around traceparent/tracestate parsing order (#106)

* split extract_context, fix tracestate parsing, adjust to spec around traceparent/tracestate parsing order

* Add test for dict based header

* up v

Author: krynju

src/api/Project.toml

@@ -1,7 +1,7 @@
 name = "OpenTelemetryAPI"
 uuid = "4f63ef3d-5940-44e7-a611-2d97696cffc9"
 authors = ["Jun Tian <tianjun.cpp@gmail.com>"]
-version = "0.5.0"
+version = "0.5.1"
 
 [deps]
 Dates = "ade2ca70-3891-5945-98fb-dc099432e06a"

src/api/src/propagator/trace_context_textmap_propagator.jl

@@ -43,29 +43,80 @@ TRACEPARENT_HEADER_FORMAT =
     r"^[ \t]*(?P<version>[0-9a-f]{2})-(?P<trace_id>[0-9a-f]{32})-(?P<span_id>[0-9a-f]{16})-(?P<trace_flag>[0-9a-f]{2})(?P<rest>-.*)?[ \t]*$"
 
 function extract_context(
-    carrier::Union{
-        AbstractVector{<:Pair{<:AbstractString,<:AbstractString}},
-        AbstractDict{<:AbstractString,<:AbstractString},
-    },
+    carrier::AbstractDict{<:AbstractString,<:AbstractString},
     propagator::TraceContextTextMapPropagator,
     ctx::Context = current_context(),
 )
-    trace_id, span_id, trace_flag, trace_state = nothing, nothing, nothing, TraceState()
-    for (k, v) in carrier
-        if k == "traceparent"
-            m = match(TRACEPARENT_HEADER_FORMAT, v)
-            if !isnothing(m)
-                if m["version"] == "00" && isnothing(m["rest"])
-                    trace_id = parse(TraceIdType, m["trace_id"], base = 16)
-                    span_id = parse(SpanIdType, m["span_id"], base = 16)
-                    trace_flag = parse(TraceFlag, m["trace_flag"])
-                end
+    trace_id = nothing
+    span_id = nothing
+    trace_flag = nothing
+    trace_state = TraceState()
+
+    carrier_keys = collect(keys(carrier))
+
+    traceparent_idx = findfirst(k -> lowercase(k) == "traceparent", carrier_keys)
+
+    if traceparent_idx !== nothing
+        m = match(TRACEPARENT_HEADER_FORMAT, carrier[carrier_keys[traceparent_idx]])
+        if m !== nothing && m["version"] == "00" && m["rest"] === nothing
+            trace_id = parse(TraceIdType, m["trace_id"], base = 16)
+            span_id = parse(SpanIdType, m["span_id"], base = 16)
+            trace_flag = parse(TraceFlag, m["trace_flag"])
+
+            # we only look for/parse tracestate if traceparent is ok
+            tracestate_idx = findfirst(k -> lowercase(k) == "tracestate", carrier_keys)
+            if tracestate_idx !== nothing
+                trace_state = parse(TraceState, carrier[carrier_keys[tracestate_idx]])
             end
-        elseif k == "tracestate"
-            trace_state = parse(TraceState, v)
         end
     end
-    if isnothing(trace_id) || isnothing(span_id) || isnothing(trace_flag)
+
+    return _extract_context(trace_id, span_id, trace_flag, trace_state, propagator, ctx)
+end
+
+function extract_context(
+    carrier::AbstractVector{<:Pair{<:AbstractString,<:AbstractString}},
+    propagator::TraceContextTextMapPropagator,
+    ctx::Context = current_context(),
+)
+    trace_id = nothing
+    span_id = nothing
+    trace_flag = nothing
+    trace_state = TraceState()
+
+    traceparent_idx = findfirst(kv -> lowercase(kv[1]) == "traceparent", carrier)
+
+    if traceparent_idx !== nothing
+        m = match(TRACEPARENT_HEADER_FORMAT, carrier[traceparent_idx][2])
+        if m !== nothing && m["version"] == "00" && m["rest"] === nothing
+            trace_id = parse(TraceIdType, m["trace_id"], base = 16)
+            span_id = parse(SpanIdType, m["span_id"], base = 16)
+            trace_flag = parse(TraceFlag, m["trace_flag"])
+
+            # we only look for/parse tracestate if traceparent is ok
+            tracestate_idx = findfirst(kv -> lowercase(kv[1]) == "tracestate", carrier)
+            if tracestate_idx !== nothing
+                trace_state = parse(TraceState, carrier[tracestate_idx][2])
+            end
+        end
+    end
+
+    return _extract_context(trace_id, span_id, trace_flag, trace_state, propagator, ctx)
+end
+
+function _extract_context(
+    trace_id,
+    span_id,
+    trace_flag,
+    trace_state,
+    propagator::TraceContextTextMapPropagator,
+    ctx::Context = current_context(),
+)
+    return if (
+        trace_id === nothing || 
+        span_id === nothing || 
+        trace_flag === nothing
+    )
         ctx
     else
         merge(

src/api/src/trace/tracer_basic.jl

@@ -107,7 +107,9 @@ function Base.show(io::IO, ts::TraceState)
 end
 
 function Base.parse(::Type{TraceState}, s::AbstractString)
-    TraceState((Pair(split(kv, '=')...) for kv in split(s, ','))...)
+    kvs = split(s, ',')
+    fkvs = filter(kv -> match(r"^[^=]*=[^=]*$", kv) !== nothing, kvs)
+    TraceState((Pair(split(kv, '=')...) for kv in fkvs)...)
 end
 
 """

src/api/test/propagator.jl

@@ -20,6 +20,24 @@
         @test sc.trace_flag == TraceFlag(false)
     end
 
+
+    header = Dict{String,String}(
+        "traceparent" => "00-$(string(test_trace_id, base=16, pad=32))-$(string(test_span_id,base=16,pad=16))-00",
+        "tracestate" => tracestate_value,
+    )
+    with_context(; x = 123) do
+        ctx = extract_context(header)
+        @test ctx[:x] == 123
+        @test ctx |> current_span |> span_status == SpanStatus(SPAN_STATUS_UNSET)
+
+        sc = span_context(ctx)
+        @test sc.trace_id == test_trace_id
+        @test sc.span_id == test_span_id
+        @test string(sc.trace_state) ==
+              string(TraceState("foo" => "1", "bar" => "2", "baz" => "3"))
+        @test sc.trace_flag == TraceFlag(false)
+    end
+
     header = Dict("Content-Type" => "text/plain")
     with_span("test") do
         inject_context!(header)

src/api/test/trace.jl

@@ -18,6 +18,10 @@
             @test haskey(ts, :foo)
             @test !haskey(ts, "^foo-_*/bar")
             @test ts[:foo] == "bar3"
+
+            @test isempty(parse(TraceState, ""))
+            @test !isempty(parse(TraceState, "a=b,c=d"))
+            @test !isempty(parse(TraceState, "a=b,ss=dwdw=dwdwd,c=d"))
         end
     end