search

jaegertracing / jaeger-client-cpp

🛑 This library is DEPRECATED!
https://jaegertracing.io/
Apache License 2.0
138 stars 126 forks source link

Added Support for W3C traceparent/tracestate propagation #255

Closed tobiasstadler closed 3 years ago

tobiasstadler commented 3 years ago

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

AppVeyorBot commented 3 years ago

:white_check_mark: Build jaeger-client-cpp 165 completed (commit https://github.com/jaegertracing/jaeger-client-cpp/commit/50d272a82a by @tobiasstadler)

tobiasstadler commented 3 years ago

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 commented 3 years ago

:white_check_mark: Build jaeger-client-cpp 166 completed (commit https://github.com/jaegertracing/jaeger-client-cpp/commit/596f828f95 by @tobiasstadler)

AppVeyorBot commented 3 years ago

:white_check_mark: Build jaeger-client-cpp 167 completed (commit https://github.com/jaegertracing/jaeger-client-cpp/commit/cf6a8948fb by @tobiasstadler)

AppVeyorBot commented 3 years ago

:white_check_mark: Build jaeger-client-cpp 168 completed (commit https://github.com/jaegertracing/jaeger-client-cpp/commit/8b60484b8d by @tobiasstadler)

AppVeyorBot commented 3 years ago

:white_check_mark: Build jaeger-client-cpp 169 completed (commit https://github.com/jaegertracing/jaeger-client-cpp/commit/6cd47258eb by @tobiasstadler)

miry commented 3 years ago

Hi @tobiasstadler ,

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

  1. I am not sure how I can set propagation through config. Pls help me
  2. 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 commented 3 years ago

Hi @tobiasstadler ,

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

  1. I am not sure how I can set propagation through config. Pls help me
  2. 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 commented 3 years ago

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

AppVeyorBot commented 3 years ago

:white_check_mark: Build jaeger-client-cpp 170 completed (commit https://github.com/jaegertracing/jaeger-client-cpp/commit/66de646a9b by @tobiasstadler)

miry commented 3 years ago

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 commented 3 years ago

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

tobiasstadler commented 3 years ago

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

seabaylea commented 3 years ago

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

miry commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

AppVeyorBot commented 3 years ago

:x: Build jaeger-client-cpp 177 failed (commit https://github.com/jaegertracing/jaeger-client-cpp/commit/8848375e1b by @tobiasstadler)

AppVeyorBot commented 3 years ago

:white_check_mark: Build jaeger-client-cpp 181 completed (commit https://github.com/jaegertracing/jaeger-client-cpp/commit/475ea9af40 by @tobiasstadler)

AppVeyorBot commented 3 years ago

:white_check_mark: Build jaeger-client-cpp 182 completed (commit https://github.com/jaegertracing/jaeger-client-cpp/commit/b4ff338d1a by @tobiasstadler)

yurishkuro commented 3 years ago

🎉

miry commented 3 years ago

👍

tobiasstadler commented 3 years ago

Thank You @yurishkuro!

timmysilv commented 3 years ago

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

yurishkuro commented 3 years ago

released v0.7.0