Fix log sdk builder (#1486)

[marcalff]

Fixes #1486

Changes

Implement builders for log SDK.

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed (no API change)

[codecov]

Codecov Report

Merging #1524 (5724cea) into main (3af512e) will increase coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1524      +/-   ##
==========================================
+ Coverage   84.59%   84.65%   +0.07%     
==========================================
  Files         156      156              
  Lines        4794     4794              
==========================================
+ Hits         4055     4058       +3     
+ Misses        739      736       -3     

Impacted Files	Coverage Δ
ext/src/http/client/curl/http_client_curl.cc	81.07% <0.00%> (+1.14%)	⬆️

[owent]

Just want to discuss, do we need a standalone factor for every processor?Or it's more simple to use just one factory to create different type of process and all return std::unique_ptr<LogProcessor>?

[lalitb]

Just want to discuss, do we need a standalone factor for every processor?Or it's more simple to use just one factory to create different type of process and all return std::unique_ptr<LogProcessor>?

I don't have strong preference for either approach - both looks good.

[marcalff]

Just want to discuss, do we need a standalone factor for every processor?Or it's more simple to use just one factory to create different type of process and all return std::unique_ptr<LogProcessor>?

@owent , @lalitb Good question, as it is another possible design.

Assuming something like:

class LogProcessorFactory
{
  static std::unique_ptr<LogProcessor> CreateSimpleLogProcessor(...);
  static std::unique_ptr<LogProcessor> CreateMultiLogProcessor(...);
  static std::unique_ptr<LogProcessor> CreateBatchLogProcessor(...);
};

there is only one class, with builders for each flavor of log processor.

This design works here for log processor, but does not for other cases.

For example applying the same pattern to trace exporters, it leads to:

• a class TraceExporterFactory
• that depends on the Jaeger exporter
• that depends on the OStream exporter
• that depends on the OTLP HTTP exporter
• that depends on the OTLP GRPC exporter

Because of this class, there is no longer a way to link only a few exporters the application owner wants in the final binary.

Instead, every exporter has to be linked in the final application, which is not good.

So, to avoid confusion for the user of this code, the design here sticks to always the same pattern,
which is easy to remember, and also works well with libraries dependencies:

To get a instance of class XXX, call XXXFactory::Create()

The XXXFactory class is provided in the same library as class XXX, which makes it possible to use XXX but not YYY.

[owent]

LGTM, Could you please also add a standalone unit test so that we can ensure factory headers of OTLP exporters do not include any protobuf header?

[lalitb]

So, to avoid confusion for the user of this code, the design here sticks to always the same pattern,
which is easy to remember, and also works well with libraries dependencies:
To get a instance of class XXX, call XXXFactory::Create()

Agree, the current approach looks good.

[marcalff]

LGTM, Could you please also add a standalone unit test so that we can ensure factory headers of OTLP exporters do not include any protobuf header?

Indeed, I should have known from traces.

Thanks, unit test added.

[lalitb]

LGTM, Could you please also add a standalone unit test so that we can ensure factory headers of OTLP exporters do not include any protobuf header?

@owent - Can you approve the changes if the recent unit tests look good? Thanks.

[owent]

LGTM