Otlp Retry Part2 - Introduce transmission handler

[vishweshbankwar]

Towards #1779 #4791

Changes

This is part2 which introduces a OtlpExporterTransmissionHandler<T> class. This class will be used as a base class to implement retry strategies (i.e. InMemory and PersistentStorage).

NOTE This PR does not change the existing functionality of otlp exporters. Actual retry strategies will be implemented in follow up PRs, for details 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)

[codecov]

Codecov Report

Attention: Patch coverage is 91.07143% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 83.27%. Comparing base (6250307) to head (375b211).
Report is 96 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5367      +/-   ##
==========================================
- Coverage   83.38%   83.27%   -0.11%     
==========================================
  Files         297      279      -18     
  Lines       12531    11961     -570     
==========================================
- Hits        10449     9961     -488     
+ Misses       2082     2000      -82     

Flag	Coverage Δ
unittests	?
unittests-Solution-Experimental	83.23% <89.28%> (?)
unittests-Solution-Stable	83.16% <91.07%> (?)

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

Files	Coverage Δ
...mentation/ExportClient/BaseOtlpHttpExportClient.cs	94.59% <ø> (-5.41%)	⬇️
...tation/OpenTelemetryProtocolExporterEventSource.cs	89.65% <100.00%> (-10.35%)	⬇️
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs	93.87% <100.00%> (+0.26%)	⬆️
....Exporter.OpenTelemetryProtocol/OtlpLogExporter.cs	91.17% <100.00%> (+2.28%)	⬆️
...porter.OpenTelemetryProtocol/OtlpMetricExporter.cs	76.66% <100.00%> (-6.67%)	⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs	88.46% <100.00%> (-2.72%)	⬇️
...on/Transmission/OtlpExporterTransmissionHandler.cs	81.48% <81.48%> (ø)

... and 43 files with indirect coverage changes

[utpilla]

Why does this need to be a method of its own? A transmission handler could simply override OnSubmitRequestFailure to implement this logic.

[vishweshbankwar]

This is a common method that can be used by implementations. OnSubmitRequestFailure would call this method along with other logic.

[CodeBlanch]

Should we rename this TryRetryRequest now that we have TrySubmitRequest? Both have similar mechanics this one just returns more details.

[vishweshbankwar]

done

[utpilla]

Why is this suppression only required for this event?

[vishweshbankwar]

I was discussing the same with @Yun-Ting offline. AOT compatibility test fails without this. We have this in few other places too like

opentelemetry-dotnet/src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs

Lines 208 to 221 in 5bcc805

	#if NET6_0_OR_GREATER
	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "Parameters to this method are primitive and are trimmer safe.")]
	#endif
	[Event(32, Message = "'{0}' exporting to '{1}' dropped '{2}' item(s) due to buffer full.", Level = EventLevel.Warning)]
	public void ExistsDroppedExportProcessorItems(string exportProcessorName, string exporterName, long droppedCount)
	{
	this.WriteEvent(32, exportProcessorName, exporterName, droppedCount);
	}
	#if NET6_0_OR_GREATER
	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "Parameters to this method are primitive and are trimmer safe.")]
	#endif
	[Event(33, Message = "Measurements from Instrument '{0}', Meter '{1}' will be ignored. Reason: '{2}'. Suggested action: '{3}'", Level = EventLevel.Warning)]
	public void MetricInstrumentIgnored(string instrumentName, string meterName, string reason, string fix)

@Yun-Ting - Could you please open an issue to investigate this?

[CodeBlanch]

The suppression is safe to remove.

The example above doing...

this.WriteEvent(32, exportProcessorName, exporterName, droppedCount);

That is calling https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.tracing.eventsource.writeevent?view=net-8.0#system-diagnostics-tracing-eventsource-writeevent(system-int32-system-diagnostics-tracing-eventsource-eventsourceprimitive()) which is decorated like this:

        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
                   Justification = EventSourceSuppressMessage)]
        protected void WriteEvent(int eventId, params EventSourcePrimitive[] args)

This method here is just going to call https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.tracing.eventsource.writeevent?view=net-8.0#system-diagnostics-tracing-eventsource-writeevent(system-int32-system-string) which should be fine.

[vishweshbankwar]

There are two methods in the example above using suppressiom. one of them does not use https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.tracing.eventsource.writeevent?view=net-8.0#system-diagnostics-tracing-eventsource-writeevent(system-int32-system-diagnostics-tracing-eventsource-eventsourceprimitive()) and that one is also decorated.

Removed: But see this from previous PR

I was able to fix it only by adding the suppression. So something has changed at runtime level.

[CodeBlanch]

Can you link me to the one you found with UnconditionalSuppressMessage not using that params form of WriteEvent? I'm looking around but not finding it. Mostly I'm just curious to try and figure out what's going on 😄

[CodeBlanch]

One suggestion/comment but LGTM