Fix trace sdk builder 1393 by marcalff · Pull Request #1471 · open-telemetry/opentelemetry-cpp

marcalff · 2022-06-30T19:54:31Z

(Last edit 2022-07-12)

Changes

Implement Trace SDK builders, to avoid exposing SDK details to the application.
This is needed to deploy opentelemetry-cpp as a shared library.

Fixed memory ownership of InMemorySpanExporter in examples.

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 (no API change)
Added design documentation

[Trace SDK] Implement builders (open-telemetry#1393)

codecov · 2022-06-30T20:09:13Z

Codecov Report

Merging #1471 (40217d5) into main (c224633) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1471   +/-   ##
=======================================
  Coverage   84.73%   84.73%           
=======================================
  Files         155      155           
  Lines        4787     4787           
=======================================
  Hits         4056     4056           
  Misses        731      731

Fixed bazel build, clang-format.

Format cleanup.

Use std::unique_ptr instead of nostd::unique_ptr

Continued, implemented: - JaegerExporterFactory, - InMemorySpanExporterFactory, - RandomIdGeneratorFactory.

Added doc. Makefile format.

Misc cleanup.

marcalff · 2022-07-01T17:53:39Z

(Last edit 2022-07-08)

This PR is now ready for review.

Remaining tasks:

run again all examples locally

Reviewers:

Please start by reading docs/cpp-sdk-factory-design.md
Please check the Bazel BUILD files in particular (I never used Bazel)
To discuss: std versus nostd

Regards,
-- Marc

lalitb

Very nicely done. Thanks for the PR. Looks good with few (very) minor comments. Hope it would be fine to merge @owent 's async changes first :)

Implemented code review comments: - reworded docs/cpp-sdk-factory-design.md, - re organized includes, - added TracerContextFactory. Also: - renamed Build() methods to Create(), for consistency with HttpClientFactory::Create(), - added doxygen comments, - added CHANGELOG entry.

marcalff · 2022-07-08T15:50:15Z

Very nicely done. Thanks for the PR. Looks good with few (very) minor comments. Hope it would be fine to merge @owent 's async changes first :)

Thanks.

The async changes have priority, I will deal with any merge conflict, these should be trivial to resolve anyway.

One remaining point to discuss, that I am not sure of, is whether Factory classes should use:

std::shared_ptr
nostd::shared_ptr
because this is part of the application owner / SDK interface.

@lalitb , @owent , @ThomsonTan , @esigo any input ?

owent · 2022-07-08T16:09:25Z

Very nicely done. Thanks for the PR. Looks good with few (very) minor comments. Hope it would be fine to merge @owent 's async changes first :)

Thanks.

The async changes have priority, I will deal with any merge conflict, these should be trivial to resolve anyway.

One remaining point to discuss, that I am not sure of, is whether Factory classes should use:

std::shared_ptr

nostd::shared_ptr
because this is part of the application owner / SDK interface.

@lalitb , @owent , @ThomsonTan , @esigo any input ?

Or std::unique_ptr so users can convert it to any implementation of shared_ptr?

Conflicts: CHANGELOG.md exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter.h

Implement code review comments: - Factory methods should return a unique_ptr. - Used TracerContextFactory in examples.

marcalff · 2022-07-09T17:33:26Z

Or std::unique_ptr so users can convert it to any implementation of shared_ptr?

Thanks, good point: a factory method should return a unique_ptr anyway.
Fixed.

lalitb · 2022-07-09T18:05:15Z

@owent Can you please review/approve it once you have time? Thanks :)

owent · 2022-07-10T02:58:47Z

@owent Can you please review/approve it once you have time? Thanks :)

Sorry for something else in company take my bandwidth. I will review this it in 2 or 3 days.

lalitb · 2022-07-10T03:55:55Z

@owent Can you please review/approve it once you have time? Thanks :)

Sorry for something else in company take my bandwidth. I will review this it in 2 or 3 days.

Thanks. That should be fine :)

owent

Could you please add a standalone unit test for OTLP exporters and ES exporters which only include factory headers and do not depend headers of protobuf? I think it's helpful to keep factory heads clean in the future.

Implement code review comments: - avoid copy of resource parameters (used const reference) - added unit tests to verify build sanity - moved JsonBytesMappingKind and HttpRequestContentType to their own header, to avoid the dependency on protobuf from otlp_http_client.h

Fixed makefiles.

marcalff · 2022-07-12T09:55:08Z

Could you please add a standalone unit test for OTLP exporters and ES exporters which only include factory headers and do not depend headers of protobuf? I think it's helpful to keep factory heads clean in the future.

Good idea, agreed.

For the ES Log exporter, this will be done with the fix for issue #1486, as this change scope is for traces only.

Added a unit test for the OTLP GRPC exporter factory, which was clean.

Added a unit test for the OTLP HTTP exporter factory, which was broken.

Fixed my moving some declarations from otlp_http_client.h to new file otlp_http.h,
to avoid transitive includes on protobuf caused by otlp_http_client.h

While at it, also made sure the OTLP HTTP exporter does not expose nlohmann/json.

Thanks for the review.

Unit test cleanup

owent

LGTM

marcalff · 2022-07-13T08:32:38Z

@lalitb, @ThomsonTan Do not merge yet.

It turns out the library name for the in memory exporter is visible externally after all,
in cmake/opentelemetry-cpp-config.cmake.in

opentelemetry-cpp::in_memory_span_exporter - Imported target of opentelemetry-cpp::in_memory_span_exporter

Reverting un needed changes in exporter/memory/CMakeLists.txt, will notify when ready to merge.

CMake cleanup. Reverted the renaming of library opentelemetry_exporter_in_memory to opentelemetry_exporter_memory_trace. The name opentelemetry_exporter_in_memory is used externally, in cmake/opentelemetry-cpp-config.cmake.in

Format makefile.

marcalff · 2022-07-13T12:20:58Z

@lalitb, @ThomsonTan Do not merge yet.

It turns out the library name for the in memory exporter is visible externally after all, in cmake/opentelemetry-cpp-config.cmake.in

opentelemetry-cpp::in_memory_span_exporter - Imported target of opentelemetry-cpp::in_memory_span_exporter

Reverting un needed changes in exporter/memory/CMakeLists.txt, will notify when ready to merge.

@lalitb @ThomsonTan @owent
Now resolved, sorry for the last minute changes.

Ok to merge for my part.

Fixed grammar in documentation.

marcalff and others added 2 commits June 30, 2022 21:26

Draft - work in progress

6861368

[Trace SDK] Implement builders (open-telemetry#1393)

Merge branch 'open-telemetry:main' into fix_trace_sdk_builder_1393

af9a5f2

marcalff added 6 commits June 30, 2022 22:44

[Trace SDK] Implement builders (open-telemetry#1393)

777a4fc

Fixed bazel build, clang-format.

[Trace SDK] Implement builders (open-telemetry#1393)

356ef47

Format cleanup.

[Trace SDK] Implement builders (open-telemetry#1393)

250bcde

Use std::unique_ptr instead of nostd::unique_ptr

[Trace SDK] Implement builders (open-telemetry#1393)

7041253

Continued, implemented: - JaegerExporterFactory, - InMemorySpanExporterFactory, - RandomIdGeneratorFactory.

[Trace SDK] Implement builders (open-telemetry#1393)

a176c4c

Added doc. Makefile format.

[Trace SDK] Implement builders (open-telemetry#1393)

c94c174

Misc cleanup.

marcalff marked this pull request as ready for review July 1, 2022 17:53

marcalff requested a review from a team July 1, 2022 17:53

Merge branch 'main' into fix_trace_sdk_builder_1393

89fbfbe

lalitb approved these changes Jul 6, 2022

View reviewed changes

Comment thread docs/cpp-sdk-factory-design.md Outdated

Comment thread examples/batch/main.cc Outdated

Comment thread exporters/memory/CMakeLists.txt Outdated

Comment thread sdk/src/trace/tracer_provider_factory.cc

marcalff added 2 commits July 9, 2022 11:37

Merge branch 'main' into fix_trace_sdk_builder_1393

8a4eadc

Conflicts: CHANGELOG.md exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter.h

[Trace SDK] Implement builders (open-telemetry#1393)

f8fe74d

Implement code review comments: - Factory methods should return a unique_ptr. - Used TracerContextFactory in examples.

Merge branch 'open-telemetry:main' into fix_trace_sdk_builder_1393

83e7a1c

owent reviewed Jul 11, 2022

View reviewed changes

owent mentioned this pull request Jul 11, 2022

[Metrics] Add OTLP http metric exporter #1487

Merged

10 tasks

marcalff added 2 commits July 12, 2022 11:00

[Trace SDK] Implement builders (open-telemetry#1393)

9cd6bb9

Fixed makefiles.

Merge branch 'open-telemetry:main' into fix_trace_sdk_builder_1393

2ad543a

owent reviewed Jul 12, 2022

View reviewed changes

Comment thread exporters/otlp/test/otlp_http_exporter_factory_test.cc

Comment thread exporters/otlp/test/otlp_grpc_exporter_factory_test.cc

[Trace SDK] Implement builders (open-telemetry#1393)

9077623

Unit test cleanup

owent approved these changes Jul 12, 2022

View reviewed changes

Merge branch 'main' into fix_trace_sdk_builder_1393

08fb578

marcalff added 2 commits July 13, 2022 11:13

[Trace SDK] Implement builders (open-telemetry#1393)

1dea7f3

CMake cleanup. Reverted the renaming of library opentelemetry_exporter_in_memory to opentelemetry_exporter_memory_trace. The name opentelemetry_exporter_in_memory is used externally, in cmake/opentelemetry-cpp-config.cmake.in

[Trace SDK] Implement builders (open-telemetry#1393)

d4b651e

Format makefile.

marcalff mentioned this pull request Jul 15, 2022

[Examples] Memory ownership issues in InMemorySpanExporter usage example. #1473

Closed

ThomsonTan reviewed Jul 15, 2022

View reviewed changes

Comment thread docs/cpp-sdk-factory-design.md Outdated

[Trace SDK] Implement builders (open-telemetry#1393)

40217d5

Fixed grammar in documentation.

lalitb merged commit e708de5 into open-telemetry:main Jul 15, 2022

This was referenced Jul 27, 2022

Review and enhance tracing support for currency service (C++) open-telemetry/opentelemetry-demo#52

Closed

REQUEST: New membership for @marcalff open-telemetry/community#1122

Closed

yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022

Fix trace sdk builder 1393 (open-telemetry#1471)

2b60051

marcalff deleted the fix_trace_sdk_builder_1393 branch July 4, 2023 07:13

Conversation

marcalff commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

marcalff commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lalitb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marcalff commented Jul 8, 2022

Uh oh!

owent commented Jul 8, 2022

Uh oh!

marcalff commented Jul 9, 2022

Uh oh!

lalitb commented Jul 9, 2022

Uh oh!

owent commented Jul 10, 2022

Uh oh!

lalitb commented Jul 10, 2022

Uh oh!

owent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marcalff commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owent left a comment

Choose a reason for hiding this comment

Uh oh!

marcalff commented Jul 13, 2022

Uh oh!

marcalff commented Jul 13, 2022

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marcalff commented Jun 30, 2022 •

edited

Loading

codecov Bot commented Jun 30, 2022 •

edited

Loading

marcalff commented Jul 1, 2022 •

edited

Loading

marcalff commented Jul 12, 2022 •

edited

Loading