[sdk] Provide better concurrency modes

[reyang]

This is following the same design as push/pull metrics exporter:

opentelemetry-dotnet/src/OpenTelemetry/Metrics/ExportModesAttribute.cs

Lines 9 to 10 in 3cff6db

	[AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = true)]
	public sealed class ExportModesAttribute : Attribute

Exporter (also processors, samplers, etc.) authors can optionally provide additional hints so the SDK can better serve the need:

[ConcurrencyModes(ConcurrencyModes.Multithreaded | ConcurrencyModes.Reentrant)]
internal class MyExporter : BaseExporter<LogRecord>
{
    ...
}

In addition, I envision that Console Exporters can leverage this to provide synchronization across multiple instances, so a log exporter and metrics exporter won't have race condition and cause garbled text in stdout:

[ConcurrencyModes(ConcurrencyModes.Global)]
internal class ConsoleExporter<T> : BaseExporter<T>
{
    ...
}

[reyang]

This is to show how the attributes can be used, once folks agree with the direction, I'll revert the example code and update the changelog.

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 7 lines in your changes missing coverage. Please review.

Project coverage is 85.67%. Comparing base (6250307) to head (c5b6dd1).
Report is 259 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5643      +/-   ##
==========================================
+ Coverage   83.38%   85.67%   +2.29%     
==========================================
  Files         297      255      -42     
  Lines       12531    11051    -1480     
==========================================
- Hits        10449     9468     -981     
+ Misses       2082     1583     -499     

Flag	Coverage Δ
unittests	?
unittests-Project-Experimental	85.62% <66.66%> (?)
unittests-Project-Stable	85.60% <66.66%> (?)

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

Files	Coverage Δ
src/OpenTelemetry/SimpleExportProcessor.cs	85.71% <82.35%> (-14.29%)	⬇️
src/OpenTelemetry/ConcurrencyModesAttribute.cs	0.00% <0.00%> (ø)

... and 114 files with indirect coverage changes

[CodeBlanch]

We removed Global for now should we also remove Reentrant? Doesn't seem to be used at the moment.

[reyang]

My gut feeling is no. Multithreaded and Reentrant are closely related, the existing implementation (which uses lock(obj)) implies that the exporter supports reentrancy but not multithreading (which is buggy IMHO).

[reyang]

No strong feelings though, I can scope this out and add it later once we figured out how to correctly handle reentrancy.

[CodeBlanch]

Interesting. So if the attribute is not specified and defaults to 0 our implied default is essentially SingleThreaded | Reentrant?

Should the defined Reentrant flag then negate the default behavior?

public enum ConcurrencyModes : byte
{
    /// <summary>
    /// Nonreentrant, the component cannot be invoked recursively without resulting
    /// in a deadlock or infinite loop.
    /// </summary>
    Nonreentrant = 0b1,

    /// <summary>
    /// Multithreaded, the component can be invoked concurrently across
    /// multiple threads.
    /// </summary>
    Multithreaded = 0b10,
}

[reyang]

Interesting. So if the attribute is not specified and defaults to 0 our implied default is essentially SingleThreaded | Reentrant?

Maybe. I personally consider this as a bug in the current SDK implementation.

Should the defined Reentrant flag then negate the default behavior?

I guess no, better to treat it as a bug, then figure out how to fix it in a non-breaking way.

[reyang]

My thinking - if the exporter doesn't have the attribute at all, fall back to the old (and buggy) behavior, if the exporter has the attribute, start to enforce the correct behavior.

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

[eerhardt]

@reyang @CodeBlanch - I see this got closed due to inactivity. Do we have a plan for removing the private reflection in the GenevaExporter?

[reyang]

@reyang @CodeBlanch - I see this got closed due to inactivity. Do we have a plan for removing the private reflection in the GenevaExporter?

@CodeBlanch would you follow up?

[CodeBlanch]

@eerhardt Still working on the plan. I've been exploring some other possible APIs/alternatives. Hoping to have something to propose soon and then we'll pick a direction to have in place for the next release.