Zipkin exporter #471

lalitb · 2020-12-18T14:58:38Z

Implementation of zipkin exporter based on mapping defined here:

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk_exporters/zipkin.md

The code is ready for review while I am adding below changes in this PR:

Adding nlohmann/json as submodule to enable cmake build.
Create bazel build definition

A separate PR would be raised to add an example for how to run it against zipkin server ( under examples/zipkin)

The PR is dependent on merging of below two PRs and currently includes those changes also to enable it build successfully:
1 Sending sync request through http_client: #448
2. Http client factory: #445

codecov · 2020-12-18T15:05:35Z

Codecov Report

Merging #471 (7c62e73) into main (7706c6d) will increase coverage by 0.12%.
The diff coverage is 94.36%.

@@            Coverage Diff             @@
##             main     #471      +/-   ##
==========================================
+ Coverage   94.22%   94.34%   +0.12%     
==========================================
  Files         187      189       +2     
  Lines        8882     8949      +67     
==========================================
+ Hits         8369     8443      +74     
+ Misses        513      506       -7

Impacted Files	Coverage Δ
...nclude/opentelemetry/ext/http/client/http_client.h	`93.33% <ø> (ø)`
...include/opentelemetry/ext/http/common/url_parser.h	`91.11% <91.11%> (ø)`
...ntelemetry/ext/http/client/curl/http_client_curl.h	`92.30% <100.00%> (+10.57%)`	⬆️
ext/test/http/curl_http_test.cc	`94.62% <100.00%> (+0.27%)`	⬆️
ext/test/http/url_parser_test.cc	`100.00% <100.00%> (ø)`

exporters/zipkin/CMakeLists.txt

exporters/zipkin/include/opentelemetry/exporters/zipkin/zipkin_exporter.h

maxgolov · 2020-12-18T18:37:41Z

exporters/zipkin/src/recordable.cc

+
+const int kAttributeValueSize = 14;
+
+void Recordable::SetIds(trace::TraceId trace_id,


Is this function reusable by other exporters, would they use the same field names? id , parentId, traceId - should this be moved into common utility function within the SDK?

@mishal23 - you need to borrow this same code for ETW exporter, to populate the actual IDs in your PR.

@maxgolov - Not sure if we can move it to common. As different exporters expect these fields in different formats. E.g, Zipkin (json) as hex chars of 16/32 length, Jaeger (binary thrift) as uint64_t.

The implementation of Recordable is exporter specific. As exporter could choose different underlying storage, agree that it is unnecessary to provide a common implementation.

ext/test/http/curl_http_test.cc

Co-authored-by: Max Golovanov <max.golovanov+github@gmail.com>

exporters/zipkin/include/opentelemetry/exporters/zipkin/zipkin_exporter.h

bogdandrutu · 2021-01-13T23:38:35Z

exporters/zipkin/include/opentelemetry/exporters/zipkin/zipkin_exporter.h

+  /**
+   * Create an ZipkinExporter using the given options.
+   */
+  ZipkinExporter(const ZipkinExporterOptions &options);


Use explicit for all 1 arguments constructor. I think it is a bad practice to rely on converting constructors (https://en.cppreference.com/w/cpp/language/converting_constructor) in general unless explicitly wanted to have this behavior like std::string_view.

/cc @maxgolov @lalitb @g-easy @jsuereth I see that in other places you do not use explicit, what is the overall rule?

Agree. Will fix it. We follow google c++ style guide:
(https://google.github.io/styleguide/cppguide.html#Implicit_Conversions):
Do not define implicit conversions. Use the explicit keyword for conversion operators and single-argument constructors.

Also, would be good to add cpplint CI check to catch these error.

This is fixed for zipkin. Will have separate PR for adding explicit keyword at other places.

pyohannes · 2021-01-15T00:16:02Z

exporters/zipkin/CMakeLists.txt

+find_package(CURL REQUIRED)
+if(CURL_FOUND)
+  add_compile_definitions(WITH_CURL)
+endif()


If this REQUIRED argument is given, we can remove the if condition, as configuration will abort anyway if CURL is not found.

Thank you. Have removed the REQUIRED argument, as it should be possible to run zipkin with other http client libraries.

pyohannes · 2021-01-15T00:23:25Z

exporters/zipkin/include/opentelemetry/exporters/zipkin/zipkin_exporter.h

+/**
+ * The Zipkin exporter exports span data in JSON format as expected by Zipkin
+ */
+class ZipkinExporter final : public trace_sdk::SpanExporter, public http_client::EventHandler


Why do you derive from EventHandler here?

Good catch. It is not needed, and so removed.

pyohannes · 2021-01-15T00:28:44Z

exporters/zipkin/src/zipkin_exporter.cc

+  {
+    return sdk::trace::ExportResult::kFailure;
+  }
+  auto session = http_session_manager_->CreateSession(url_parser_.host_, url_parser_.port_);


Why do we create a new session for each call to Export? I see sessions are stored in the session manager. When are those sessions cleaned up? Will this cause an implicit memory leak (all past sessions being kept in the session manager)?

Session object is confusing, as there is no actual session created for HTTP ( being stateless). I have simplified the http client library as part of #448 , and using that here:

HttpClientSync httpClient; auto result = httpClient.Post(url, data, headers); // GET request if (result){ auto response = result.GetResponse(); } else { std::cout << result.GetSessionState(); }

Muchene · 2021-02-12T19:23:48Z

@lalitb
`zipkin::ZipkinExporterOptions options;
options.endpoint="http://" + ip_str + ":" + std::to_string(port) + "/api/v2/spans";
std::cout << "options " << options.endpoint << std::endl;
new_trace = (struct mva_telemetry_trace *) mal_calloc(1,sizeof(*new_trace));

const size_t max_export_batch_size = 10;
const size_t max_queue_size = max_export_batch_size * 2 + 5;
const std::chrono::milliseconds schedule_delay_millis = std::chrono::milliseconds(30000);
auto exporter = std::unique_ptr<sdktrace::SpanExporter>( new zipkin::ZipkinExporter(options));
auto processor = std::shared_ptr<sdktrace::SpanProcessor>(new sdktrace::BatchSpanProcessor(
  std::move(exporter), max_queue_size, schedule_delay_millis, max_export_batch_size));
auto provider = nostd::shared_ptr<opentelemetry::trace::TracerProvider>(
    new sdktrace::TracerProvider(processor));
// Set the global trace provider
opentelemetry::trace::Provider::SetTracerProvider(provider);`

Segfault occurs when creating the exporter.

lalitb · 2021-02-15T06:46:43Z

@lalitb
`zipkin::ZipkinExporterOptions options;
options.endpoint="http://" + ip_str + ":" + std::to_string(port) + "/api/v2/spans";
std::cout << "options " << options.endpoint << std::endl;
new_trace = (struct mva_telemetry_trace *) mal_calloc(1,sizeof(*new_trace));
const size_t max_export_batch_size = 10;
const size_t max_queue_size = max_export_batch_size * 2 + 5;
const std::chrono::milliseconds schedule_delay_millis = std::chrono::milliseconds(30000);
auto exporter = std::unique_ptr<sdktrace::SpanExporter>( new zipkin::ZipkinExporter(options));
auto processor = std::shared_ptr<sdktrace::SpanProcessor>(new sdktrace::BatchSpanProcessor(
  std::move(exporter), max_queue_size, schedule_delay_millis, max_export_batch_size));
auto provider = nostd::shared_ptr<opentelemetry::trace::TracerProvider>(
    new sdktrace::TracerProvider(processor));
// Set the global trace provider
opentelemetry::trace::Provider::SetTracerProvider(provider);`
Segfault occurs when creating the exporter.

Thanks for the snippet @Muchene . though I am not able to reproduce the issue, the problem could be related to order of member initialization. For now, I have changed the order of member variable definition in ZipkinExporter to be in consistent with the order of their initialization in constructor initializers. Hopefully it helps. Do let me know once you try it again.

Muchene · 2021-02-16T21:39:14Z

@lalitb Will try and let you know. One more thing

void Recordable::AddEvent(nostd::string_view name, core::SystemTimestamp timestamp, const common::KeyValueIterable &attributes) noexcept { nlohmann::json annotations = nlohmann::json::object(); // empty object attributes.ForEachKeyValue([&](nostd::string_view key, common::AttributeValue value) noexcept { PopulateAttribute(annotations, key, value); return true; }); span_["annotations"][name.data()]["value"] = annotations; span_["annotations"]["timestamp"] = std::chrono::duration_cast<std::chrono::milliseconds>(timestamp.time_since_epoch()).count(); }
may be wrong as annotations are expected to be an array by zipkin.

lalitb · 2021-02-25T18:36:04Z

@lalitb Will try and let you know. One more thing

void Recordable::AddEvent(nostd::string_view name, core::SystemTimestamp timestamp, const common::KeyValueIterable &attributes) noexcept { nlohmann::json annotations = nlohmann::json::object(); // empty object attributes.ForEachKeyValue([&](nostd::string_view key, common::AttributeValue value) noexcept { PopulateAttribute(annotations, key, value); return true; }); span_["annotations"][name.data()]["value"] = annotations; span_["annotations"]["timestamp"] = std::chrono::duration_cast<std::chrono::milliseconds>(timestamp.time_since_epoch()).count(); }
may be wrong as annotations are expected to be an array by zipkin.

@Muchene - Thanks for reporting the issue. Have chaged the annotations to array now as expected by Zipkin Server. Please let know if you still see any issues here.

lalitb · 2021-03-02T14:19:53Z

@open-telemetry/cpp-approvers - For this PR, all the comments have been resolved, and any incremental change can be applied later. It's planned to be merged today eod unless there are any concerns raised.

reyang · 2021-03-02T20:50:17Z

exporters/CMakeLists.txt

@@ -1,3 +1,17 @@
+# Copyright 2021, OpenTelemetry Authors


Somehow I haven't noticed this before...
Do we need to include the actual year, or we can do something simpler like https://github.com/open-telemetry/opentelemetry-dotnet/blob/381908c089aac4fa9a2056d6bbf465b9853b81b8/src/OpenTelemetry/AssemblyInfo.cs#L2?

Not sure where I copied from. Have fixed it now. Thanks for the comment.

exporters/zipkin/include/opentelemetry/exporters/zipkin/zipkin_exporter.h

reyang · 2021-03-02T20:56:31Z

exporters/zipkin/src/recordable.cc

+      attribute[key.data()].push_back(val);
+    }
+  }
+  else if (nostd::holds_alternative<nostd::span<const nostd::string_view>>(value))


Just curious about the perf implication of these if-else checks, not sure if a small jump table would make it faster. Not a blocker for this PR though.

Yes, we are doing these variant checks at multiple places, would be better to use dispatch table throughout. Will add a ticket for same.

reyang

LGTM.

reyang · 2021-03-02T20:58:06Z

exporters/zipkin/src/zipkin_exporter.cc

+
+}  // namespace zipkin
+}  // namespace exporter
+OPENTELEMETRY_END_NAMESPACE


nit: I noticed some files have missed the blank line before EOF.

Good catch. Have added blank line at all missing places.

…_exporter.h Co-authored-by: Reiley Yang <reyang@microsoft.com>

lalitb added 7 commits December 18, 2020 15:58

zipkin changes

8fd81b0

reformat

a532b72

fix cmake format

861da22

fix build

586d5c9

remove formats

babbc5d

remove console debugs

5570d18

remove debug

1039f4c

lalitb requested a review from a team December 18, 2020 14:58

maxgolov reviewed Dec 18, 2020

View reviewed changes

exporters/zipkin/CMakeLists.txt Show resolved Hide resolved

maxgolov reviewed Dec 18, 2020

View reviewed changes

exporters/zipkin/include/opentelemetry/exporters/zipkin/zipkin_exporter.h Outdated Show resolved Hide resolved

maxgolov reviewed Dec 18, 2020

View reviewed changes

ext/test/http/curl_http_test.cc Outdated Show resolved Hide resolved

lalitb and others added 2 commits December 21, 2020 22:57

Update exporters/zipkin/CMakeLists.txt

fd5a5b6

Co-authored-by: Max Golovanov <max.golovanov+github@gmail.com>

fix endpoint

d4f22e3

ThomsonTan reviewed Dec 21, 2020

View reviewed changes

exporters/zipkin/include/opentelemetry/exporters/zipkin/zipkin_exporter.h Outdated Show resolved Hide resolved

lalitb added 5 commits December 22, 2020 21:05

add noop response

0fbe3af

Merge branch 'master' into zipkin

6d3e445

fix nullptr for http errors

cdf5dc4

remove otlp stuff

19d789f

fix sync mode

84b0458

bogdandrutu reviewed Jan 13, 2021

View reviewed changes

pyohannes reviewed Jan 15, 2021

View reviewed changes

merge conflict

93beff6

Base automatically changed from master to main January 26, 2021 00:46

lalitb and others added 5 commits January 26, 2021 21:47

merge conflict

81049c7

fixes

791e2a1

fix

aff76e2

fix

dcc758b

fix spell

b3e96c7

lalitb added 3 commits February 15, 2021 10:35

Merge branch 'main' into zipkin

590bbcf

fix

b54304d

Merge branch 'zipkin' of github.com:lalitb/opentelemetry-cpp into zipkin

ad1506e

Merge branch 'main' into zipkin

7450085

lalitb added 5 commits February 18, 2021 18:56

Merge branch 'main' into zipkin

11ffb1e

Merge branch 'main' into zipkin

e6e333f

Merge branch 'main' into zipkin

d4c7217

fix annotations as per specs

653d084

Merge branch 'zipkin' of github.com:lalitb/opentelemetry-cpp into zipkin

e0057f3

Merge branch 'main' into zipkin

6c89e4f

Merge branch 'main' into zipkin

0eb9ce4

reyang reviewed Mar 2, 2021

View reviewed changes

exporters/zipkin/include/opentelemetry/exporters/zipkin/zipkin_exporter.h Outdated Show resolved Hide resolved

reyang reviewed Mar 2, 2021

View reviewed changes

exporters/zipkin/include/opentelemetry/exporters/zipkin/zipkin_exporter.h Outdated Show resolved Hide resolved

reyang reviewed Mar 2, 2021

View reviewed changes

reyang approved these changes Mar 2, 2021

View reviewed changes

reyang reviewed Mar 2, 2021

View reviewed changes

lalitb and others added 6 commits March 3, 2021 17:49

Update exporters/zipkin/include/opentelemetry/exporters/zipkin/zipkin…

4633712

…_exporter.h Co-authored-by: Reiley Yang <reyang@microsoft.com>

Update exporters/zipkin/include/opentelemetry/exporters/zipkin/zipkin…

359a385

…_exporter.h Co-authored-by: Reiley Yang <reyang@microsoft.com>

fix comments

051fccf

add changelog

acb1040

fix md

f97e6ca

fix

fa6826a

lalitb merged commit 71d5b1d into open-telemetry:main Mar 3, 2021

lalitb mentioned this pull request Apr 19, 2021

Implement Zipkin exporter #76

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zipkin exporter #471

Zipkin exporter #471

lalitb commented Dec 18, 2020

codecov bot commented Dec 18, 2020 •

edited

Loading

maxgolov Dec 18, 2020

lalitb Dec 22, 2020 •

edited

Loading

ThomsonTan Jan 27, 2021

bogdandrutu Jan 13, 2021

lalitb Jan 14, 2021 •

edited

Loading

lalitb Jan 27, 2021

pyohannes Jan 15, 2021

lalitb Jan 27, 2021

pyohannes Jan 15, 2021

lalitb Jan 27, 2021

pyohannes Jan 15, 2021

lalitb Jan 27, 2021

Muchene commented Feb 12, 2021 •

edited

Loading

lalitb commented Feb 15, 2021

Muchene commented Feb 16, 2021 •

edited

Loading

lalitb commented Feb 25, 2021

lalitb commented Mar 2, 2021 •

edited

Loading

reyang Mar 2, 2021

lalitb Mar 3, 2021

reyang Mar 2, 2021

lalitb Mar 3, 2021

reyang left a comment

reyang Mar 2, 2021

lalitb Mar 3, 2021


		const int kAttributeValueSize = 14;

		void Recordable::SetIds(trace::TraceId trace_id,

Zipkin exporter #471

Zipkin exporter #471

Conversation

lalitb commented Dec 18, 2020

codecov bot commented Dec 18, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

lalitb Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb Jan 14, 2021 • 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

Choose a reason for hiding this comment

Muchene commented Feb 12, 2021 • edited Loading

lalitb commented Feb 15, 2021

Muchene commented Feb 16, 2021 • edited Loading

lalitb commented Feb 25, 2021

lalitb commented Mar 2, 2021 • 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

reyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 18, 2020 •

edited

Loading

lalitb Dec 22, 2020 •

edited

Loading

lalitb Jan 14, 2021 •

edited

Loading

Muchene commented Feb 12, 2021 •

edited

Loading

Muchene commented Feb 16, 2021 •

edited

Loading

lalitb commented Mar 2, 2021 •

edited

Loading