Skip to content

tracer: Added tests for casting to HttpTracerImpl#5250

Merged
jmarantz merged 1 commit intoenvoyproxy:masterfrom
jmarantz:cast-to-zipkin-tracer
Dec 9, 2018
Merged

tracer: Added tests for casting to HttpTracerImpl#5250
jmarantz merged 1 commit intoenvoyproxy:masterfrom
jmarantz:cast-to-zipkin-tracer

Conversation

@jmarantz
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz commented Dec 9, 2018

Description: follow-up to #5249 adding tests for being able to cast to HttpTracerImpl.
Risk Level: low
Testing: //test/server:server_test
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz jmarantz changed the title Added tests for casting to HttpTracerImpl. tracer: Added tests for casting to HttpTracerImpl Dec 9, 2018
Copy link
Copy Markdown
Member

@venilnoronha venilnoronha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks.

EXPECT_EQ(nullptr, dynamic_cast<Tracing::HttpNullTracer*>(&server_->httpContext().tracer()));
EXPECT_EQ(nullptr, dynamic_cast<Tracing::HttpNullTracer*>(tracer()));

// Note: there is no ZipkingTracerImpl object;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably expose the underlying driver somehow and dynamic cast to that, but not sure it's worth it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought of that and also felt like that was overkill. Really the way to go deeper on this is to have a zipkin integration test that shows zipkin syntax coming out of the server. But for the resolution of #5241 I really wanted something that just looked at the tracer field.

@jmarantz jmarantz merged commit fd24599 into envoyproxy:master Dec 9, 2018
@jmarantz jmarantz deleted the cast-to-zipkin-tracer branch December 9, 2018 18:31
fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants