Otlp Retry Part1 - Refactor ExportClients

[vishweshbankwar]

Towards #1779 #4791
Design discussion issue #

Changes

This PR is part1 of multiple PRs which will bring retry functionality to otlp exporters in case of transient errors.

1. It updates IExportClient interface definition to return a custom response object. Response object will be either of type ExportClientHttpResponse or ExportClientGrpcResponse based on the protocol used to send data i.e. http protobuf or grpc.
2. Exporter implementations are updated accordingly.
3. Removing the existing OtlpExporterTransmissionHandler class that was added in Add base class for implementing retry strategies in otlp exporter #4872. I plan to add an updated version of this in upcoming PRs.

NOTE This PR does not change the existing functionality of otlp exporters. For further details on what will be upcoming changes, take a look at this POC #5311

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes
• Changes in public API reviewed (if applicable)

[CodeBlanch]

LGTM

[rajkumar-rangaraj]

LGTM.

Tests are missing for few of the properties in ExportClientResponse. Are you planning to cover in the follow up PRs?

[codecov]

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (6250307) 83.38% compared to head (56fc828) 82.87%.
Report is 84 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5335      +/-   ##
==========================================
- Coverage   83.38%   82.87%   -0.51%     
==========================================
  Files         297      276      -21     
  Lines       12531    12003     -528     
==========================================
- Hits        10449     9948     -501     
+ Misses       2082     2055      -27     

Flag	Coverage Δ
unittests	?
unittests-Solution-Experimental	82.84% <76.47%> (?)
unittests-Solution-Stable	82.85% <76.47%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...mentation/ExportClient/BaseOtlpGrpcExportClient.cs	85.00% <100.00%> (+0.78%)	⬆️
...mentation/ExportClient/ExportClientGrpcResponse.cs	100.00% <100.00%> (ø)
...entation/ExportClient/OtlpGrpcTraceExportClient.cs	92.30% <100.00%> (-0.55%)	⬇️
....Exporter.OpenTelemetryProtocol/OtlpLogExporter.cs	100.00% <100.00%> (+11.11%)	⬆️
...porter.OpenTelemetryProtocol/OtlpMetricExporter.cs	90.00% <100.00%> (+6.66%)	⬆️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs	91.17% <100.00%> (ø)
...ementation/ExportClient/OtlpGrpcLogExportClient.cs	84.61% <50.00%> (+6.04%)	⬆️
...tation/ExportClient/OtlpGrpcMetricsExportClient.cs	84.61% <50.00%> (+6.04%)	⬆️
...mentation/ExportClient/BaseOtlpHttpExportClient.cs	94.59% <75.00%> (-5.41%)	⬇️
...mentation/ExportClient/ExportClientHttpResponse.cs	66.66% <66.66%> (ø)
... and 1 more

... and 41 files with indirect coverage changes

[alanwest]

👍

[utpilla]

Why does this not use the timeout value from the OtlpExporterOptions?

[vishweshbankwar]

good point - It is because user can override it with their own HttpClient instance via HttpClientFactory. So using the timeout value from the client itself and not options.

[utpilla]

Do we have a way to detect if the user actually configured timeout through OtlpExporter options?

[vishweshbankwar]

No - even if they are not using custom client. We initialize default client using timeout value set via options.

opentelemetry-dotnet/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs

Lines 82 to 88 in e4b08ac

	this.HttpClientFactory = this.DefaultHttpClientFactory = () =>
	{
	return new HttpClient
	{
	Timeout = TimeSpan.FromMilliseconds(this.TimeoutMilliseconds),
	};
	};

So in both the cases we will have the correct deadline here.

Edit: In case of custom client, the timeout set within the client will take precedence over timeout set in options.

[utpilla]

It would be good to add this as a comment.

[vishweshbankwar]

Sure, I can add it in my next PR if thats ok.