Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add OTLP Metric Exporter Factory and OtlpGrpcClient #1606

Merged

Conversation

owent
Copy link
Member

@owent owent commented Sep 9, 2022

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

Fixes #1603
Fixes #1605

Changes

  • Add OtlpGrpcMetricExporterFactory and OtlpHttpMetricExporterFactory.
  • Fix compatibility of OTLP log unit tests for MSVC.
  • Add OtlpGrpcClient

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

@owent owent requested a review from a team September 9, 2022 15:55
@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #1606 (30e4586) into main (951768a) will decrease coverage by 0.07%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1606      +/-   ##
==========================================
- Coverage   85.26%   85.20%   -0.06%     
==========================================
  Files         156      156              
  Lines        4978     4978              
==========================================
- Hits         4244     4241       -3     
- Misses        734      737       +3     
Impacted Files Coverage Δ
ext/src/http/client/curl/http_client_curl.cc 80.31% <0.00%> (-1.13%) ⬇️

@owent owent changed the title [WIP] Add OTLP Metric Exporter Factory and OtlpGrpcClient Add OTLP Metric Exporter Factory and OtlpGrpcClient Sep 9, 2022
@lalitb
Copy link
Member

lalitb commented Sep 9, 2022

@owent - you may want to fix the conflict from #1601 for this PR :)

@owent owent force-pushed the fix_1603_and_add_otlp_metric_factory branch from 83c88ae to 9580025 Compare September 9, 2022 18:39
@owent
Copy link
Member Author

owent commented Sep 9, 2022

@owent - you may want to fix the conflict from #1601 for this PR :)

Done

Copy link
Member

@esigo esigo left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for the PR :)

@lalitb
Copy link
Member

lalitb commented Sep 12, 2022

Did a quick validations on the binaries generated from this PR.

It seems that example_otlp_grpc_log still has dependency on both libopentelemetry_exporter_otlp_grpc.so and libopentelemetry_exporter_otlp_grpc_log.so. Should that be fixed?

$ ldd example_otlp_grpc_log
        linux-vdso.so.1 (0x00007fff86bf5000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f4b4ef1b000)
        libcommon_logs_foo_library.so => /tmp/owent/opentelemetry-cpp/build/examples/common/logs_foo_library/libcommon_logs_foo_library.so (0x00007f4b4eeb6000)
        libopentelemetry_exporter_otlp_grpc.so => /tmp/owent/opentelemetry-cpp/build/exporters/otlp/libopentelemetry_exporter_otlp_grpc.so (0x00007f4b4edff000)
        libopentelemetry_exporter_otlp_grpc_log.so => /tmp/owent/opentelemetry-cpp/build/exporters/otlp/libopentelemetry_exporter_otlp_grpc_log.so (0x00007f4b4ed48000)
        libopentelemetry_trace.so => /tmp/owent/opentelemetry-cpp/build/sdk/src/trace/libopentelemetry_trace.so (0x00007f4b4ec4a000)
        libopentelemetry_logs.so => /tmp/owent/opentelemetry-cpp/build/sdk/src/logs/libopentelemetry_logs.so (0x00007f4b4eb63000)
        libopentelemetry_exporter_otlp_grpc_client.so => /tmp/owent/opentelemetry-cpp/build/exporters/otlp/libopentelemetry_exporter_otlp_grpc_client.so (0x00007f4b4e5e2000)
        libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f4b4e400000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f4b4e3e5000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f4b4e1f3000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f4b4f1d1000)
        libopentelemetry_otlp_recordable.so => /tmp/owent/opentelemetry-cpp/build/exporters/otlp/libopentelemetry_otlp_recordable.so (0x00007f4b4dec1000)
        libopentelemetry_common.so => /tmp/owent/opentelemetry-cpp/build/sdk/src/common/libopentelemetry_common.so (0x00007f4b4deae000)
        libopentelemetry_resources.so => /tmp/owent/opentelemetry-cpp/build/sdk/src/resource/libopentelemetry_resources.so (0x00007f4b4ddd1000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f4b4dc82000)
        libssl.so.1.1 => /lib/x86_64-linux-gnu/libssl.so.1.1 (0x00007f4b4dbef000)
        libcrypto.so.1.1 => /lib/x86_64-linux-gnu/libcrypto.so.1.1 (0x00007f4b4d917000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f4b4d911000)

While example_otlp_grpc is rightly dependent on only libopentelemetry_exporter_otlp_grpc.so.

$ ldd ./example_otlp_grpc
        linux-vdso.so.1 (0x00007fff1a3e1000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f6543907000)
        libcommon_foo_library.so => /tmp/owent/opentelemetry-cpp/build/examples/common/foo_library/libcommon_foo_library.so (0x00007f65438ac000)
        libopentelemetry_exporter_otlp_grpc.so => /tmp/owent/opentelemetry-cpp/build/exporters/otlp/libopentelemetry_exporter_otlp_grpc.so (0x00007f65437f5000)
        libopentelemetry_trace.so => /tmp/owent/opentelemetry-cpp/build/sdk/src/trace/libopentelemetry_trace.so (0x00007f65436f9000)
        libopentelemetry_exporter_otlp_grpc_client.so => /tmp/owent/opentelemetry-cpp/build/exporters/otlp/libopentelemetry_exporter_otlp_grpc_client.so (0x00007f6543176000)
        libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f6542f94000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f6542f79000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f6542d87000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f6543b93000)
        libopentelemetry_otlp_recordable.so => /tmp/owent/opentelemetry-cpp/build/exporters/otlp/libopentelemetry_otlp_recordable.so (0x00007f6542a57000)
        libopentelemetry_common.so => /tmp/owent/opentelemetry-cpp/build/sdk/src/common/libopentelemetry_common.so (0x00007f6542a42000)
        libopentelemetry_resources.so => /tmp/owent/opentelemetry-cpp/build/sdk/src/resource/libopentelemetry_resources.so (0x00007f6542965000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f6542816000)
        libssl.so.1.1 => /lib/x86_64-linux-gnu/libssl.so.1.1 (0x00007f6542783000)
        libcrypto.so.1.1 => /lib/x86_64-linux-gnu/libcrypto.so.1.1 (0x00007f65424ad000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f65424a5000)

@lalitb
Copy link
Member

lalitb commented Sep 12, 2022

Fix compatibility of OTLP log unit tests for MSVC.

What is the compatibility issue here? Thanks.

@owent
Copy link
Member Author

owent commented Sep 12, 2022

Did a quick validations on the binaries generated from this PR.

It seems that example_otlp_grpc_log still has dependency on both libopentelemetry_exporter_otlp_grpc.so and libopentelemetry_exporter_otlp_grpc_log.so. Should that be fixed?

$ ldd example_otlp_grpc_log
        linux-vdso.so.1 (0x00007fff86bf5000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f4b4ef1b000)
        libcommon_logs_foo_library.so => /tmp/owent/opentelemetry-cpp/build/examples/common/logs_foo_library/libcommon_logs_foo_library.so (0x00007f4b4eeb6000)
        libopentelemetry_exporter_otlp_grpc.so => /tmp/owent/opentelemetry-cpp/build/exporters/otlp/libopentelemetry_exporter_otlp_grpc.so (0x00007f4b4edff000)
        libopentelemetry_exporter_otlp_grpc_log.so => /tmp/owent/opentelemetry-cpp/build/exporters/otlp/libopentelemetry_exporter_otlp_grpc_log.so (0x00007f4b4ed48000)
        libopentelemetry_trace.so => /tmp/owent/opentelemetry-cpp/build/sdk/src/trace/libopentelemetry_trace.so (0x00007f4b4ec4a000)
        libopentelemetry_logs.so => /tmp/owent/opentelemetry-cpp/build/sdk/src/logs/libopentelemetry_logs.so (0x00007f4b4eb63000)
        libopentelemetry_exporter_otlp_grpc_client.so => /tmp/owent/opentelemetry-cpp/build/exporters/otlp/libopentelemetry_exporter_otlp_grpc_client.so (0x00007f4b4e5e2000)
        libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f4b4e400000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f4b4e3e5000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f4b4e1f3000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f4b4f1d1000)
        libopentelemetry_otlp_recordable.so => /tmp/owent/opentelemetry-cpp/build/exporters/otlp/libopentelemetry_otlp_recordable.so (0x00007f4b4dec1000)
        libopentelemetry_common.so => /tmp/owent/opentelemetry-cpp/build/sdk/src/common/libopentelemetry_common.so (0x00007f4b4deae000)
        libopentelemetry_resources.so => /tmp/owent/opentelemetry-cpp/build/sdk/src/resource/libopentelemetry_resources.so (0x00007f4b4ddd1000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f4b4dc82000)
        libssl.so.1.1 => /lib/x86_64-linux-gnu/libssl.so.1.1 (0x00007f4b4dbef000)
        libcrypto.so.1.1 => /lib/x86_64-linux-gnu/libcrypto.so.1.1 (0x00007f4b4d917000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f4b4d911000)

While example_otlp_grpc is rightly dependent on only libopentelemetry_exporter_otlp_grpc.so.

$ ldd ./example_otlp_grpc
        linux-vdso.so.1 (0x00007fff1a3e1000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f6543907000)
        libcommon_foo_library.so => /tmp/owent/opentelemetry-cpp/build/examples/common/foo_library/libcommon_foo_library.so (0x00007f65438ac000)
        libopentelemetry_exporter_otlp_grpc.so => /tmp/owent/opentelemetry-cpp/build/exporters/otlp/libopentelemetry_exporter_otlp_grpc.so (0x00007f65437f5000)
        libopentelemetry_trace.so => /tmp/owent/opentelemetry-cpp/build/sdk/src/trace/libopentelemetry_trace.so (0x00007f65436f9000)
        libopentelemetry_exporter_otlp_grpc_client.so => /tmp/owent/opentelemetry-cpp/build/exporters/otlp/libopentelemetry_exporter_otlp_grpc_client.so (0x00007f6543176000)
        libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f6542f94000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f6542f79000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f6542d87000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f6543b93000)
        libopentelemetry_otlp_recordable.so => /tmp/owent/opentelemetry-cpp/build/exporters/otlp/libopentelemetry_otlp_recordable.so (0x00007f6542a57000)
        libopentelemetry_common.so => /tmp/owent/opentelemetry-cpp/build/sdk/src/common/libopentelemetry_common.so (0x00007f6542a42000)
        libopentelemetry_resources.so => /tmp/owent/opentelemetry-cpp/build/sdk/src/resource/libopentelemetry_resources.so (0x00007f6542965000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f6542816000)
        libssl.so.1.1 => /lib/x86_64-linux-gnu/libssl.so.1.1 (0x00007f6542783000)
        libcrypto.so.1.1 => /lib/x86_64-linux-gnu/libcrypto.so.1.1 (0x00007f65424ad000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f65424a5000)

Yes, but libopentelemetry_exporter_otlp_grpc_log.so and libopentelemetry_exporter_otlp_grpc.so do not link grpc anymore.

@owent
Copy link
Member Author

owent commented Sep 12, 2022

Fix compatibility of OTLP log unit tests for MSVC.

What is the compatibility issue here? Thanks.

When compiling with the latest MSVC (17.3) , the std::string array can not convert to opentelemetry::common::AttributeValue .
It seems to have some problem to convrert to span<string_view> implicitly.

@lalitb
Copy link
Member

lalitb commented Sep 12, 2022

Yes, but libopentelemetry_exporter_otlp_grpc_log.so and libopentelemetry_exporter_otlp_grpc.so do not link grpc anymore.

Yes, I can see opentelemetry_exporter_otlp_grpc_client adding the dependency on grpc which seems to be fine. But was just thinking that the actual solution could have been to remove libopentelemetry_exporter_otlp_grpc.so dependency for example_otlp_grpc_log ?

@lalitb
Copy link
Member

lalitb commented Sep 12, 2022

Yes, but libopentelemetry_exporter_otlp_grpc_log.so and libopentelemetry_exporter_otlp_grpc.so do not link grpc anymore.

>Yes, I can see opentelemetry_exporter_otlp_grpc_client adding the dependency on grpc which seems to be fine. But was just thinking that the actual solution could have been to remove libopentelemetry_exporter_otlp_grpc.so dependency for example_otlp_grpc_log ?

Please ignore this. I just realized it is possible for an instrumentation library to have a dependency on both log and trace exporters.

@lalitb
Copy link
Member

lalitb commented Sep 12, 2022

Another nit comment, now that the libgrpc.a and libprotobuf.a are statically added in opentelemetry_exporter_otlp_grpc_client.so, we can remove the gRPC dependency from otlp exporter example -

opentelemetry_trace opentelemetry_exporter_otlp_grpc gRPC::grpc++)

Not blocker for this PR as if required it can done later in #1598

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM with nit comments. Thanks for the fix.

…ory`.

+ Fix compatibility of OTLP log unit tests for MSVC.

Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
@owent owent force-pushed the fix_1603_and_add_otlp_metric_factory branch from 4581b85 to 30e4586 Compare September 13, 2022 02:23
@owent
Copy link
Member Author

owent commented Sep 13, 2022

LGTM with nit comments. Thanks for the fix.

Thanks and gRPC::grpc++ is removed from OTLP examples now.

@lalitb lalitb merged commit 0b7d4ab into open-telemetry:main Sep 13, 2022
@owent owent deleted the fix_1603_and_add_otlp_metric_factory branch October 4, 2022 06:47
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants