Add OTLP/HTTP+JSON Protocol exporter by owent · Pull Request #810 · open-telemetry/opentelemetry-cpp

owent · 2021-05-30T15:25:29Z

Fixes # 803

Changes

Implementation of HTTP(OTLP/HTTP+JSON Protocol) exporter.

~~This is a uncompleted PR and it's used to discuss the details of API and changes for now. I will add tests and CHANGELOG some time later.~~

Almost finished.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

codecov · 2021-05-30T15:33:27Z

Codecov Report

Merging #810 (eaca36e) into main (12946c8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #810   +/-   ##
=======================================
  Coverage   95.50%   95.50%           
=======================================
  Files         156      156           
  Lines        6619     6619           
=======================================
  Hits         6321     6321           
  Misses        298      298

lalitb · 2021-05-31T07:18:10Z

I add a OtlpHttpExporter , should we rename OtlpExporter into OtlpGrpcExporter and rename otlp_exporter.h|.cpp into otlp_exporter.h/.cpp ? It's a break change.

I agree we should rename OtlpExporter into OtlpGrpcExporter. We are not GA yet, so breaking changes are expected, and should be acceptable.

lalitb

LGTM. Thanks for the contribution :)

lalitb · 2021-06-03T14:27:06Z

+  /**
+   * Create an OtlpHttpExporter using the given options.
+   */
+  OtlpHttpExporter(const OtlpHttpExporterOptions &options);


Nit - can we make this explicit. I think this is missing for OtlpGrpcExporter too which also should be modified.

My negligence. I will add explicit later and add some tests.

ThomsonTan · 2021-06-03T15:00:53Z

+        strip_prefix = "grpc-1.33.2",
        urls = [
-            "https://github.com/grpc/grpc/archive/v1.28.0.tar.gz",
+            "https://github.com/grpc/grpc/archive/v1.33.2.tar.gz",


Why upgrade this to 1.33? I think 1.34 is used in our CI.

opentelemetry-cpp/ci/setup_grpc.sh

Line 16 in 0f6199f

git clone --depth=1 -b v1.34.0 https://github.com/grpc/grpc

I think 1.33 is the only version support both bazel 4+ and legacy gcc 4.8.
setup_grpc.sh is only used when using cmake, not bazel.
I found grpc 1.34 upgrade abseil-cpp to 2020923.2 and it uses something gcc 4.8 do not support.
I submit another PR #666 and try to upgrade gRPC but failed on legacy toolchain. Maybe some one can help to switch the
grpc to old version or just disable OTLP Exporters when using bazel?I'm not familar with bazel.

I assume for Windows, while eventually getting to prebuilt DLL / SDK, we can most likely use whatever recent version of gRPC from vcpkg? Mine right now is 1.37.0

C:\work\opentelemetry-cpp\tools\vcpkg>vcpkg list grpc grpc:x64-windows 1.37.0#2 An RPC library and framework

I hope that one works with Visual Studio 2015 Update 2 and above.

I tested several toolchains with different versions of gRPC, and here is the result:

When using bazel to build opentelemetry-cpp

gRPC 1.33(which upgrade protobuf to 1.34) support bazel 4

gRPC 1.28-1.32 only support bazel 3

gRPC 1.28-1.33 support legacy gcc 4.8 while 1.34 or upper (which upgrade abseil to 20200923.X) don't

When using cmake to build opentelemetry-cpp

Just like bazel, and the official support of vcpkg is gcc 6+.

The protobuf (3.15) in the latest release of vcpkg(2021.05.12) do not support the latest MSVC 1929(VS 16.10)

Maybe we need wait for newer version to work with MSVC 1929(VS 16.10) or upper.
Related to opentelemetry::proto::common::v1::_KeyValue_default_instance_: illegal initialization of constinit entity with a non-constant expression (C++20) #819 Fixes to compile SDK with Visual Studio 2019 Update 16.10 (C++20) #820

Right now, we have legacy gcc 4.8 in CI matrix, so I just update gRPC in bazel to 1.33 for compatiability. Maybe it can be upgraded to newer version in the future and in another PR.

ThomsonTan · 2021-06-04T02:23:12Z

 default. This can be changed with first argument from command-line, for example:
-`./example_otlp gateway.docker.internal:4317`.
+`./example_otlp_grpc gateway.docker.internal:4317` and
+`./example_otlp_http gateway.docker.internal:4317`..


nit: 2 dots at the end of line 47.

maxgolov · 2021-06-04T02:48:57Z

@@ -1,4 +1,4 @@
-if(WITH_OTLP)
+if(WITH_OTLP_GRPC OR WITH_OTLP_HTTP)


Do we really need that level of granularity, can one flag build both?

I think it is rare for one build to have both, so 2 flags make sense. Or we could introduce a third flag WITH_OTLP to include both but then it seems over complicated.

Sometimes we want only either WITH_OTLP_GRPC or WITH_OTLP_HTTP .
Set WITH_OTLP=ON will try to turn WITH_OTLP_GRPC on when grpc is found and turn WITH_OTLP_HTTP on when both libcurl and nlohmann_json are found in this PR.
The WITH_OTLP option is just for backward compatibility, and the actually options take effect is WITH_OTLP_GRPC and WITH_OTLP_HTTP now.

owent · 2021-06-04T03:30:41Z

~~Please do not merge this PR right now. I will add test codes and support for application/x-protobuf later.~~

All jobs done, could you please review the changes again? @maxgolov @ThomsonTan @lalitb

lalitb

LGTM. Thanks for adding the tests.

Signed-off-by: owent <admin@owent.net>

…tlp_exporter.cpp` to `OtlpGrpcExporter`, `OtlpGrpcExporterOptions`, `otlp_grpc_exporter.h`, `otlp_grpc_exporter.cpp` + Update gRPC to 1.33.2(which is the first version support bazel 4+ and the last version support gcc 4.8) Signed-off-by: owent <admin@owent.net>

Signed-off-by: owent <admin@owent.net>

owent requested a review from a team May 30, 2021 15:25

lalitb reviewed May 31, 2021

View reviewed changes

Comment thread exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter.h Outdated

lalitb mentioned this pull request Jun 1, 2021

HTTPClient : Support for full URL argument instead of host & port #812

Closed

lalitb approved these changes Jun 3, 2021

View reviewed changes

ThomsonTan reviewed Jun 3, 2021

View reviewed changes

ThomsonTan reviewed Jun 4, 2021

View reviewed changes

ThomsonTan approved these changes Jun 4, 2021

View reviewed changes

maxgolov reviewed Jun 4, 2021

View reviewed changes

maxgolov approved these changes Jun 4, 2021

View reviewed changes

owent force-pushed the otlp_http_exporter branch from 1e7054d to 69e4f24 Compare June 5, 2021 04:42

owent mentioned this pull request Jun 5, 2021

Update gRPC #666

Closed

3 tasks

lalitb reviewed Jun 7, 2021

View reviewed changes

Comment thread exporters/otlp/test/otlp_http_exporter_test.cc

lalitb approved these changes Jun 7, 2021

View reviewed changes

owent added 7 commits June 7, 2021 19:28

Add OTLP/HTTP+JSON Protocol exporter

5e91f3e

Signed-off-by: owent <admin@owent.net>

Remove old ResponseHandler for curl client

dcc66c6

Signed-off-by: owent <admin@owent.net>

Add test for otlp http exporter

5e502df

Signed-off-by: owent <admin@owent.net>

Add support of application/x-protobuf for OtlpHttpExporter.

2cdf438

Signed-off-by: owent <admin@owent.net>

Finish tests for OtlpHttpExporter

d4d3459

Signed-off-by: owent <admin@owent.net>

Add binary mode for otlp_http_exporter

eaca36e

Signed-off-by: owent <admin@owent.net>

owent force-pushed the otlp_http_exporter branch from 6953eb3 to eaca36e Compare June 7, 2021 11:28

lalitb merged commit f65b07a into open-telemetry:main Jun 8, 2021

owent deleted the otlp_http_exporter branch June 8, 2021 08:18

This was referenced Jun 8, 2021

Implement HTTP(OTLP/HTTP+JSON Protocol) exporters #803

Closed

Implement timeout and cancelable OtlpHttpExporter #852

Closed

johanneswuerbach mentioned this pull request Jul 13, 2021

feat(nginx): opentelemetry-cpp v1.0.0-rc3 open-telemetry/opentelemetry-cpp-contrib#45

Merged

owent mentioned this pull request Nov 24, 2021

REQUEST: New membership for @owent open-telemetry/community#920

Closed

6 tasks

lalitb mentioned this pull request Mar 22, 2022

Add owent as an Approver #1276

Merged

3 tasks

		@@ -1,4 +1,4 @@
		if(WITH_OTLP)
		if(WITH_OTLP_GRPC OR WITH_OTLP_HTTP)

Conversation

owent commented May 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

codecov Bot commented May 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

lalitb commented May 31, 2021

Uh oh!

lalitb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxgolov Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owent Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owent Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owent commented Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lalitb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

owent commented May 30, 2021 •

edited

Loading

codecov Bot commented May 30, 2021 •

edited

Loading

maxgolov Jun 4, 2021 •

edited

Loading

owent Jun 4, 2021 •

edited

Loading

owent Jun 4, 2021 •

edited

Loading

owent commented Jun 4, 2021 •

edited

Loading