Added Support for W3C traceparent/tracestate propagation #255

tobiasstadler · 2020-11-30T14:55:01Z

Which problem is this PR solving?

The cpp client does not support injection/extracting the W3C trace parent header. I implemented support for it.

Fixes #160

Signed-off-by: Tobias Stadler <[email protected]>

AppVeyorBot · 2020-11-30T15:20:23Z

✅ Build jaeger-client-cpp 165 completed (commit 50d272a82a by @tobiasstadler)

yurishkuro

Is there support for environment variable JAEGER_PROPAGATION? Per https://www.jaegertracing.io/docs/1.21/client-features/, "w3c" lowercase value is how this propagator can be selected.

yurishkuro · 2020-11-30T17:47:12Z

src/jaegertracing/propagation/HeadersConfig.h

+            jaegerBaggageHeader,
+            traceContextHeaderName,
+            traceBaggageHeaderPrefix,
+            traceContextHeaderFormat == "W3C" ? Format::W3C : Format::JAEGER);


Q: why is this uppercase "W3C"? Where would the value come from?

Via the configuration file, e.g.

headers: TraceContextHeaderFormat: w3c

yurishkuro · 2020-11-30T17:52:18Z

src/jaegertracing/Constants.h.in

@@ -31,6 +31,7 @@ static constexpr auto kSamplerTypeTagKey = "sampler.type";
 static constexpr auto kSamplerParamTagKey = "sampler.param";
 static constexpr auto kTraceContextHeaderName = "uber-trace-id";
 static constexpr auto kTracerStateHeaderName = kTraceContextHeaderName;
+static constexpr auto kW3CTraceContextHeaderName = "traceparent";


I take it you're only supporting traceparent, but not tracestate. This makes it non-compliant propagator. At minimum it should pass on the tracestate unchanged, as was done in the Java client https://github.com/jaegertracing/jaeger-client-java/blob/5831d9ed7b465971d6c5211e3f56371cc083541a/jaeger-core/src/main/java/io/jaegertracing/internal/propagation/TraceContextCodec.java

I will look into it

…docs/1.21/client-features/, Signed-off-by: Tobias Stadler <[email protected]>

…ables Signed-off-by: Tobias Stadler <[email protected]>

tobiasstadler · 2020-11-30T19:02:46Z

Is there support for environment variable JAEGER_PROPAGATION? Per https://www.jaegertracing.io/docs/1.21/client-features/, "w3c" lowercase value is how this propagator can be selected.

I implemented support for it

AppVeyorBot · 2020-11-30T19:28:07Z

✅ Build jaeger-client-cpp 166 completed (commit 596f828f95 by @tobiasstadler)

AppVeyorBot · 2020-12-02T20:14:03Z

✅ Build jaeger-client-cpp 167 completed (commit cf6a8948fb by @tobiasstadler)

yurishkuro · 2020-12-02T20:27:56Z

src/jaegertracing/propagation/HeadersConfig.h

+                  const std::string& jaegerBaggageHeader,
+                  const std::string& traceContextHeaderName,
+                  const std::string& traceBaggageHeaderPrefix,
+                  const std::string& traceStateHeaderName,


I find this rather confusing. The traceContextHeaderName setting is already somewhat dubious as it allows to change uber-trace-id header (which is a header defined by the Jaeger propagation spec) to something completely custom that is not covered by any spec (but still uses Jaeger header syntax - ¯\_(ツ)_/¯). Extending the same override capability to tracestate header, which is the official header of W3C spec, is in the same dubious category. I would rather not do that.

Note that it is a coincidence that "traceState" in traceContextHeaderName is the same term as W3C Trace Context - they are actually unrelated.

Inalso find it kind of dubiose but tried to stick with the existing design. It is fine for me to „hardcore“ the name

yurishkuro · 2020-12-02T20:33:06Z

src/jaegertracing/propagation/Propagator.h

-            [this, &ctx, &debugID, &baggage](const std::string& rawKey,
-                                             const std::string& value) {
+            [this, &ctx, &debugID, &baggage, &traceState](
+                const std::string& rawKey, const std::string& value) {
                const auto key = normalizeKey(rawKey);
                if (key == _headerKeys.traceContextHeaderName()) {


could we split the default Jaeger propagation header handling from traceparent handling? I.e. do not overload overridable traceContextHeaderName() use case with W3C propagation at all.

In all other Jaeger SDKs, the default Jaeger propagator and W3C propagator are implemented as independent classes.

I will have a look.

To be sure, if a user selects the w3c format but not jaeger format, should only the two w3c headers be supported or also e.g. jaeger-debug-id?

It's kind of up to you. Strictly speaking, the two extra headers jaeger-debug-id and jaeger-baggage are features of the default Jaeger propagator, so if you instead configure the B3 propagator they might be either supported on not, depending on the Jaeger client implementation. But I think it's a nice-to-have to support jaeger-debug-id in the W3C propagator since it provides an additional debugging capability to the users.

Signed-off-by: Tobias Stadler <[email protected]>

AppVeyorBot · 2020-12-06T18:52:35Z

✅ Build jaeger-client-cpp 168 completed (commit 8b60484b8d by @tobiasstadler)

AppVeyorBot · 2020-12-06T19:16:53Z

✅ Build jaeger-client-cpp 169 completed (commit 6cd47258eb by @tobiasstadler)

yurishkuro · 2020-12-07T04:06:19Z

src/jaegertracing/ConfigTest.cpp

+{
+    {
+        constexpr auto kConfigYAML = R"cfg(
+propagation_format: w3c


Is the format of the config file documented somewhere?

No, because I don't know where. Should I put it in examples/config.yml.

I don't know either. Looks like it's not documented in the README, would be good to have a reference to examples/, and then include all properties in that sample config (comment them out if necessary). But this could be done via separate PR.

yurishkuro

@mdouaihy mind taking a look?

yurishkuro · 2020-12-07T04:09:41Z

src/jaegertracing/Tracer.cpp

+    HTTPHeaderPropagator* httpHeaderPropagator;
+    if (config.propagationFormat() == propagation::Format::W3C) {
+        textPropagator =
+            new propagation::W3CTextMapPropagator(config.headers(), metrics);


Are headers() needed for W3C? If different headers are used it ceases to be W3C format.

They are needed for the jaeger-debug-id header, but they are not used for the trace parent and tracestate headers. These two headers are constants.

yurishkuro · 2020-12-07T04:12:03Z

src/jaegertracing/Tracer.h

    Tracer(const std::string& serviceName,
           const std::shared_ptr<samplers::Sampler>& sampler,
           const std::shared_ptr<reporters::Reporter>& reporter,
           const std::shared_ptr<logging::Logger>& logger,
           const std::shared_ptr<metrics::Metrics>& metrics,
-           const propagation::HeadersConfig& headersConfig,
+           const TextMapPropagator* textPropagator,


my C++ is quite rusty, but why is it that the other composable objects above are captured via shared_ptr and propagators via raw pointer?

Mine is rusty too, but I changed it to shared_ptr.

yurishkuro · 2020-12-07T04:16:19Z

src/jaegertracing/propagation/Format.cpp

+
+/*
+ * Copyright (c) 2020 Uber Technologies, Inc.
+ *


Suggested change

/*

* Copyright (c) 2020 Uber Technologies, Inc.

*

/*

* Copyright (c) 2020 The Jaeger Authors

*

yurishkuro · 2020-12-07T04:16:58Z

src/jaegertracing/propagation/Format.h

@@ -0,0 +1,28 @@
+/*
+ * Copyright (c) 2020 Uber Technologies, Inc.


Suggested change

* Copyright (c) 2020 Uber Technologies, Inc.

* Copyright (c) 2020 The Jaeger Authors

yurishkuro · 2020-12-07T04:17:26Z

src/jaegertracing/propagation/JaegerPropagator.cpp

@@ -0,0 +1,17 @@
+/*
+ * Copyright (c) 2020 Uber Technologies, Inc.


Suggested change

* Copyright (c) 2020 Uber Technologies, Inc.

* Copyright (c) 2020 The Jaeger Authors

yurishkuro · 2020-12-07T04:17:45Z

src/jaegertracing/propagation/JaegerPropagator.h

@@ -0,0 +1,158 @@
+/*
+ * Copyright (c) 2020 Uber Technologies, Inc.


Suggested change

* Copyright (c) 2020 Uber Technologies, Inc.

* Copyright (c) 2020 The Jaeger Authors

miry · 2020-12-07T11:28:37Z

Hi @tobiasstadler ,

Thank you for this work. I tried to use your PR with nginx-opentracing. here is changes.

I am not sure how I can set propagation through config. Pls help me
I set through ENV and tried different values: w3c or W3C. During my tests I dont have any headers generated.

Can you help me share how can I debug the issue (I am new for CPP world)?

tobiasstadler · 2020-12-07T12:08:43Z

Hi @tobiasstadler ,

Thank you for this work. I tried to use your PR with nginx-opentracing. here is changes.

I am not sure how I can set propagation through config. Pls help me

I set through ENV and tried different values: w3c or W3C. During my tests I dont have any headers generated.

Can you help me share how can I debug the issue (I am new for CPP world)?

Hi,
By default nginx removes all env variables from the worker process (see http://nginx.org/en/docs/ngx_core_module.html). You can either add env JAEGER_PROPAGATION; to your nginx.conf or add "propagation_format": "w3c" to your jaeger-config.json

miry · 2020-12-07T16:08:37Z

@tobiasstadler thank you. Now I see the headers. Waiting when it could be merged :)

miry · 2020-12-07T16:37:24Z

src/jaegertracing/TracerTest.cpp

+    std::ostringstream oss;
+    oss << "00";
+    oss << '-';
+    oss << std::setw(16) << std::setfill('0') << std::hex


@tobiasstadler Can you help me understand why there are 0 in the first half of the trace id? Is there limitations of Jaeger?

Nevermind, I found PR #254

Yes, the jaeger client only generates 64bit trace ids. See #254.

Signed-off-by: Tobias Stadler <[email protected]>

AppVeyorBot · 2020-12-07T19:02:40Z

✅ Build jaeger-client-cpp 170 completed (commit 66de646a9b by @tobiasstadler)

miry · 2020-12-15T00:15:29Z

~~Found in opentelemetry-go by default server expected HTTP header traceparent in downcase. Here is the related PR.~~

I checked W3C doc. It has next:

In order to increase interoperability across multiple protocols and encourage successful integration, by default vendors SHOULD keep the header name lowercase. The header name is a single word without any delimiters, for example, a hyphen (-).

~~Currently Nginx + Jaeger generates header Traceparent. Can you help me to understand where it should be fixed in nginx-opentracing side or in jaeger-cpp?~~

UPDATE: I found that it is Golang library capitalize all incoming HTTP headers.

seabaylea · 2021-01-07T16:21:16Z

@tobiasstadler thanks for your hard work! Is there anything stopping this from being merged?

tobiasstadler · 2021-01-08T07:14:34Z

@seabaylea The PR needs to be reviewed by one of the maintainers

seabaylea · 2021-01-08T10:33:36Z

@miry @yurishkuro would it be possible to get this reviewed?

miry · 2021-01-27T17:17:34Z

@tobiasstadler I am not part of the core contributors.

But I am testing your changes with ingress-nginx in our clusters.
I also waiting when this PR could be merged.

yurishkuro · 2021-01-27T17:22:38Z

@miry if you're actually using this, could you provide some feedback please, and if possible some testing results? We do not have active maintainers of this library anymore, and my C++ is very rusty.

miry · 2021-01-27T17:32:52Z

could you provide some feedback please, and if possible some testing results?

@yurishkuro Good point. We have in pipeline to run load tests and compare with original ingress-nginx.

my C++ is very rusty

I have exactly the same situation.

tobiasstadler · 2021-01-29T09:06:59Z

I did limited testing on my local machine. real world testing is appreciated.

mdouaihy · 2020-12-08T12:04:57Z

README.md

@@ -104,6 +104,7 @@ JAEGER_DISABLED _(not recommended)_ | Instructs the Configuration to return a no
 JAEGER_AGENT_HOST | The hostname for communicating with agent via UDP
 JAEGER_AGENT_PORT | The port for communicating with agent via UDP
 JAEGER_ENDPOINT | The traces endpoint, in case the client should connect directly to the Collector, like http://jaeger-collector:14268/api/traces
+JAEGER_PROPAGATION | The propagation format used by the tracer. Supported values are jaeger and w3c


Maybe we can align the documentation across programming languages. Can you use instead:
Comma separated list of formats to use for propagating the trace context. Defaults to the standard Jaeger format. Valid values are jaeger, and w3c

I would suggest not doing this in this PR, because supporting more than one format simultaneously is actually non-trivial, and not even clearly defined semantically, e.g.

do we include one or both formats in the outgoing requests?

what to do if incoming request has both formats? Especially if they are conflicting?

mdouaihy · 2020-12-08T12:09:22Z

src/jaegertracing/Config.cpp

+            _propagationFormat = propagation::Format::W3C;
+        }
+        else {
+            _propagationFormat = propagation::Format::JAEGER;


I think we should validate that propagationFormat contains the expected value (jaeger). Otherwise, either throw an exception, log or do nothing.

@yurishkuro What do you think?

mdouaihy · 2020-12-08T12:10:04Z

src/jaegertracing/Config.h

+                      baggageRestrictions,
+                      serviceName,
+                      std::vector<Tag>(),
+                      propagationFormat == "w3c" ? propagation::Format::W3C


We need to validate that propagationFormat contains the expected format (jaeger).

otherwise, we could send TOTO and the system will accept it.

We should at least fall back to jaeger format if none was provided.

@yurishkuro What do you think?

fallback on empty value is fine.

Signed-off-by: Tobias Stadler <[email protected]>

AppVeyorBot · 2021-02-07T14:13:07Z

❌ Build jaeger-client-cpp 177 failed (commit 8848375e1b by @tobiasstadler)

Signed-off-by: Tobias Stadler <[email protected]>

AppVeyorBot · 2021-02-10T08:17:49Z

✅ Build jaeger-client-cpp 181 completed (commit 475ea9af40 by @tobiasstadler)

yurishkuro · 2021-02-14T21:46:58Z

src/jaegertracing/Config.h

+        propagation::Format propagationFormat;
+        if (strPropagationFormat == "w3c") {
+            propagationFormat = propagation::Format::W3C;
+        }
+        else if (strPropagationFormat == "jaeger") {
+            propagationFormat = propagation::Format::JAEGER;
+        }
+        else {
+            std::cerr << "ERROR: unknown propagation format '"
+                      << strPropagationFormat
+                      << "', falling back to jaeger propagation format";
+            propagationFormat = propagation::Format::JAEGER;
+        }


seems like this could be simplified a bit

Suggested change

propagation::Format propagationFormat;

if (strPropagationFormat == "w3c") {

propagationFormat = propagation::Format::W3C;

}

else if (strPropagationFormat == "jaeger") {

propagationFormat = propagation::Format::JAEGER;

}

else {

std::cerr << "ERROR: unknown propagation format '"

<< strPropagationFormat

<< "', falling back to jaeger propagation format";

propagationFormat = propagation::Format::JAEGER;

}

auto propagationFormat = propagation::Format::JAEGER;

if (strPropagationFormat == "w3c") {

propagationFormat = propagation::Format::W3C;

}

else if (strPropagationFormat != "jaeger") {

std::cerr << "ERROR: unknown propagation format '"

<< strPropagationFormat

<< "', falling back to jaeger propagation format";

}

also, the same logic is repeated in Config::fromEnv(), can we combine them into a shared util function?

yurishkuro · 2021-02-14T21:52:49Z

src/jaegertracing/TracerTest.cpp

+    oss << std::setw(16) << std::setfill('0') << std::hex
+        << spanContext.spanID();
+    oss << '-';
+    oss << (spanContext.isSampled() ? "01" : "00");


I have a conceptual problem with the above code - it is essentially testing production code with a copy of the same production code. A preferred test for these is to instantiate SpanContext with known IDs and then compare its serialized form with a known string. It makes the test both simpler and more reliable.

yurishkuro · 2021-02-14T22:16:04Z

src/jaegertracing/testutils/TracerUtil.cpp

@@ -1,94 +1,102 @@
-/*


NB: this diff is due to fixing line endings from Win to Unix (+1)

Signed-off-by: Tobias Stadler <[email protected]>

AppVeyorBot · 2021-02-19T14:53:12Z

✅ Build jaeger-client-cpp 182 completed (commit b4ff338d1a by @tobiasstadler)

yurishkuro · 2021-02-26T06:11:17Z

🎉

miry · 2021-02-26T07:36:24Z

👍

tobiasstadler · 2021-02-26T07:36:30Z

Thank You @yurishkuro!

timmysilv · 2021-02-28T18:44:02Z

This is awesome! Can we get a new version (like 0.6.1) released with the latest changes?

yurishkuro · 2021-02-28T20:06:40Z

released v0.7.0

Implemented W3C traceparent propagation

63a2526

Signed-off-by: Tobias Stadler <[email protected]>

yurishkuro reviewed Nov 30, 2020

View reviewed changes

yurishkuro mentioned this pull request Nov 30, 2020

w3c trace context propagation support #160

Closed

tobiasstadler added 2 commits November 30, 2020 19:37

W3C should be lowercase as mentioned in https://www.jaegertracing.io/…

bca7ba6

…docs/1.21/client-features/, Signed-off-by: Tobias Stadler <[email protected]>

Added support for setting the propagation format via environment vari…

0d5f0e7

…ables Signed-off-by: Tobias Stadler <[email protected]>

tobiasstadler changed the title ~~Added Support for W3C traceparent propagation~~ Added Support for W3C traceparent/tracestate propagation Dec 2, 2020

yurishkuro reviewed Dec 2, 2020

View reviewed changes

tobiasstadler added 4 commits December 6, 2020 13:14

Fix assignment

918a852

Signed-off-by: Tobias Stadler <[email protected]>

Implemented Support for W3C tracestate

d5ac706

Signed-off-by: Tobias Stadler <[email protected]>

Extracted Jager porpagation and W3C propgation in their own classes

1f3ed2a

Signed-off-by: Tobias Stadler <[email protected]>

Fix build

96625d4

Signed-off-by: Tobias Stadler <[email protected]>

yurishkuro reviewed Dec 7, 2020

View reviewed changes

miry reviewed Dec 7, 2020

View reviewed changes

tobiasstadler added 2 commits December 7, 2020 19:26

Fix copyright

5ba17c5

Signed-off-by: Tobias Stadler <[email protected]>

Use shared_ptr instead of raw pointer

d0c77a3

Signed-off-by: Tobias Stadler <[email protected]>

mdouaihy suggested changes Jan 31, 2021

View reviewed changes

tobiasstadler added 2 commits February 7, 2021 14:33

Log unknown propagation formats and fall back to jaeger format

43e1376

Signed-off-by: Tobias Stadler <[email protected]>

Merge remote-tracking branch 'upstream/master' into w3c-traceparent

cc95a57

Signed-off-by: Tobias Stadler <[email protected]>

Merge remote-tracking branch 'upstream/master' into w3c-traceparent

ec3745d

Signed-off-by: Tobias Stadler <[email protected]>

yurishkuro reviewed Feb 14, 2021

View reviewed changes

tobiasstadler added 2 commits February 19, 2021 15:25

Extracted propagation format parsing

09ef71e

Signed-off-by: Tobias Stadler <[email protected]>

Use SpanContext with known IDs to simplify test

d14739a

Signed-off-by: Tobias Stadler <[email protected]>

yurishkuro merged commit c1d5795 into jaegertracing:master Feb 26, 2021

tobiasstadler deleted the w3c-traceparent branch February 26, 2021 07:36

timmysilv mentioned this pull request Mar 5, 2021

update tracing libraries for opentracing 1.6.0 kubernetes/ingress-nginx#6936

Merged

8 tasks

		@@ -0,0 +1,28 @@
		/*
		* Copyright (c) 2020 Uber Technologies, Inc.

	* Copyright (c) 2020 Uber Technologies, Inc.
	* Copyright (c) 2020 The Jaeger Authors

		@@ -0,0 +1,17 @@
		/*
		* Copyright (c) 2020 Uber Technologies, Inc.

		@@ -0,0 +1,158 @@
		/*
		* Copyright (c) 2020 Uber Technologies, Inc.

Added Support for W3C traceparent/tracestate propagation #255

Added Support for W3C traceparent/tracestate propagation #255

Conversation

tobiasstadler commented Nov 30, 2020 • edited by yurishkuro Loading

Which problem is this PR solving?

AppVeyorBot commented Nov 30, 2020

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasstadler commented Nov 30, 2020

AppVeyorBot commented Nov 30, 2020

AppVeyorBot commented Dec 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Dec 6, 2020

AppVeyorBot commented Dec 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasstadler Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miry commented Dec 7, 2020

tobiasstadler commented Dec 7, 2020

miry commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Dec 7, 2020

miry commented Dec 15, 2020 • edited Loading

seabaylea commented Jan 7, 2021

tobiasstadler commented Jan 8, 2021

seabaylea commented Jan 8, 2021

miry commented Jan 27, 2021

yurishkuro commented Jan 27, 2021

miry commented Jan 27, 2021 • edited Loading

tobiasstadler commented Jan 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Feb 7, 2021

AppVeyorBot commented Feb 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Feb 19, 2021

yurishkuro commented Feb 26, 2021

miry commented Feb 26, 2021

tobiasstadler commented Feb 26, 2021

timmysilv commented Feb 28, 2021

yurishkuro commented Feb 28, 2021

tobiasstadler commented Nov 30, 2020 •

edited by yurishkuro

Loading

yurishkuro Dec 2, 2020 •

edited

Loading

tobiasstadler Dec 7, 2020 •

edited

Loading

miry commented Dec 15, 2020 •

edited

Loading

miry commented Jan 27, 2021 •

edited

Loading