Init Http::ContextImpl::tracer_ when tracing is enabled, and test it.#5249
Init Http::ContextImpl::tracer_ when tracing is enabled, and test it.#5249mattklein123 merged 2 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Joshua Marantz <jmarantz@google.com>
objectiser
left a comment
objectiser
left a comment
There was a problem hiding this comment.
@jmarantz Tested it, and worked for me. Thanks.
Only comment is that I am not sure that the jaeger_tracing.yaml (and associated test) is required? It is essentially the same configuration as zipkin. Having it included might cause confusion with the native jaeger support.
Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
@objectiser removed the jaeger testcase, although I'm confused now what the relationship between the two is given |
|
Jaeger provides an alternative zipkin compatible backend - so from the proxy's perspective they are no different. The example is slightly different due to starting up a Jaeger instance for the backend instead of Zipkin. |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks for quickly fixing!
| options_.service_cluster_name_ = "some_cluster_name"; | ||
| options_.service_node_name_ = "some_node_name"; | ||
| EXPECT_NO_THROW(initialize("test/server/zipkin_tracing.yaml")); | ||
| EXPECT_EQ(nullptr, dynamic_cast<Tracing::HttpNullTracer*>(&server_->httpContext().tracer())); |
There was a problem hiding this comment.
It might be a slightly better test to check for the Zipkin tracer type?
There was a problem hiding this comment.
Agreed...I probably should have looked deeper. Is the zipkin tracer object exposed?
In any case I could follow up with an edit tomorrow if you want to get the fix in sooner.
There was a problem hiding this comment.
Yeah it's a concrete type just like the other tracers. If the config loads we must be compiling in support so it should be available in a header. Sure let's merge now since this is pretty broken as-is and can do a follow up.
There was a problem hiding this comment.
It doesn't seem like there's a specific Zipkin class for this but I added a test for casting to HttpTracerImpl* in #5250.
Follow-up to #5249 adding tests for being able to cast the tracer to HttpTracerImpl when zipkin is configured. Signed-off-by: Joshua Marantz <jmarantz@google.com>
…envoyproxy#5249) Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
Follow-up to envoyproxy#5249 adding tests for being able to cast the tracer to HttpTracerImpl when zipkin is configured. Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
3 participants
Description: Re-enables zipkin tracing which was broken by #4997 where I failed to hook up the constructed tracer to the Http::Context and there were no tests that caught it. I had actually done this correctly in the config_validation server, but not in the production server.
Adds a test that the null-tracer gets overridden when tracing is enabled in the config.
Risk Level: low
Testing: //test/server:server_test
Docs Changes: n/a
Release Notes: n/a
Fixes: #5241