(zipkin) adds span serializer#7119
Conversation
This commits enable sending spans in zipkin v2 formats. Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: José Carlos Chávez <jcchavezs@gmail.com>
Signed-off-by: José Carlos Chávez <jcchavezs@gmail.com>
…ffers. Signed-off-by: José Carlos Chávez <jcchavezs@gmail.com>
|
@jcchavezs needs master merge; that may fix the 'format' errors at least. |
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Thanks @jcchavezs. Can you explain a bit more in depth what the goal is here? I'm not an expert in the tracing aspects of Envoy, it looks like you're trying to make use of com_github_apache_zipkinapi to improve the serialization? In any case, this PR needs a bit of work to fix build, format and also avoid including generator proto stubs and proto files that exist in external repos.
/wait
| * the method allocateBuffer(size). | ||
| */ | ||
| SpanBuffer() {} | ||
| SpanBuffer(const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion version) |
There was a problem hiding this comment.
Should these constructor args be const&?
| private: | ||
| // We use a pre-allocated vector to improve performance | ||
| std::vector<Span> span_buffer_; | ||
| private : |
| @@ -0,0 +1,2705 @@ | |||
| // Generated by the protocol buffer compiler. DO NOT EDIT! | |||
There was a problem hiding this comment.
How come you are checking in generated code? Ideally we use our *_proto_library rules to generate this on-the-fly, we don't have any pregenerated stubs elsewhere in Envoy.
| * @return a stringified JSON. | ||
| */ | ||
| const std::string toJson() override; | ||
| const std::string toJson(const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion |
| buffer.clear(); | ||
| EXPECT_EQ(0ULL, buffer.pendingSpans()); | ||
| EXPECT_EQ("[]", buffer.toStringifiedJsonArray()); | ||
| /*EXPECT_EQ("[]", buffer.toStringifiedJsonArray());*/ |
| R"("endpoint":{"ipv4":"192.168.1.2","port":3306,)" | ||
| R"("serviceName":"my_service_name"}}]})", | ||
| span.toJson()); | ||
| span.toJson(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1)); |
| switch (collector_endpoint) { | ||
| case envoy::config::trace::v2::ZipkinConfig::HTTP_JSON span_serializer = | ||
| new HTTPSpanSerializer() break; | ||
| case envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO span_serializer = |
There was a problem hiding this comment.
All of this definitely needs fix_format..
| const std::string& collector_endpoint) { | ||
| return ReporterPtr(new ReporterImpl(driver, dispatcher, collector_endpoint)); | ||
| ReporterPtr | ||
| ReporterImpl::NewInstance(Driver& driver, Event::Dispatcher& dispatcher, |
| SpanSerializer span_serializer; | ||
|
|
||
| switch (collector_endpoint) { | ||
| case envoy::config::trace::v2::ZipkinConfig::HTTP_JSON span_serializer = |
| // | ||
| syntax = "proto3"; | ||
|
|
||
| package zipkin.proto3; |
There was a problem hiding this comment.
Why isn't this sourced from an external package?
|
@jcchavezs, sorry. Is there any reason for not iterating on top of #6985? I think you can also add these changes on top of #6985, which builds fine. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
Bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
4 participants
Description: This PR adds the serializer interface to have better cohesion in the spans serializer
Risk Level: Low, since the default is still HTTP-JSON v1 based on https://github.com/apache/incubator-zipkin-api/blob/v0.2.1/zipkin-api.yaml.
Testing: Unit testing, manual integration test with real Zipkin collector server.
Docs Changes: none yet
Release Notes: