Make BatchExportActivityProcessorOptions internal

[pellared]

Addresses #2219 (comment).

Changes

BatchExportActivityProcessorOptions is made internal.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Design discussion issue Add support for OTEL_BSP_EXPORT_* environmental variables #2219 (comment)
• Changes in public API reviewed

[codecov]

Codecov Report

Merging #2350 (8b25442) into main (a538016) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 8b25442 differs from pull request most recent head 1c3fa18. Consider uploading reports for the commit 1c3fa18 to get more accurate results

@@            Coverage Diff             @@
##             main    #2350      +/-   ##
==========================================
+ Coverage   79.95%   79.98%   +0.02%     
==========================================
  Files         231      231              
  Lines        7453     7453              
==========================================
+ Hits         5959     5961       +2     
+ Misses       1494     1492       -2     

Impacted Files	Coverage Δ
...ry/Internal/BatchExportActivityProcessorOptions.cs	83.33% <ø> (ø)
...ter.ZPages/Implementation/ZPagesActivityTracker.cs	100.00% <0.00%> (+2.85%)	⬆️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	76.47% <0.00%> (+2.94%)	⬆️

[pellared]

I ended up using InternalsVisibleTo.

Using <Compile Include= approach was leading to:

Benchmarks\EventSourceBenchmarks.cs(34,13): error CS0433: The type 'OpenTelemetrySdkEventSource' exists in both 'OpenTelemetry.Exporter.Jaeger, Version=1.0.0.0, Culture=neutral, PublicKeyToken=7bd6737fe5b67e3c' and 'OpenTelemetry.Exporter.Zipkin, Version=1.0.0.0, Culture=neutral, PublicKeyToken=7bd6737fe5b67e3c'

Side note. I also know another approach that I personally like. Making everything in the Internal directory as public. By convention, it means that this API is not stable and these are discouraged to be used. However they still can be used if anyone really needs it and does not want to copy-paste stuff. Entity Framework Core is an example where this approach is used: https://github.com/dotnet/efcore/tree/release/6.0/src/EFCore/Internal. Thanks to it we could reuse the internal code in a more straightforward way in https://github.com/open-telemetry/opentelemetry-dotnet-contrib . Yet, I do not know how to configure Microsoft.CodeAnalysis.PublicApiAnalyzers to ignore a directory (I hope it is possible). Still could always add the "internal" code into a dedicated assembly for which the analyzer would be disabled.

[cijothomas]

Given this class is implementing something as per spec, I think its safe to expose this as public.
The alternate of keeping internal and using internalvisible might cause other issues like exporters accidently accessing something it was not meant to, or hide issues affecting other exporter which don't get the same privilege as the ones in this repo.

[cijothomas]

@CodeBlanch what do you think here?
(Internal would be fine, if we can use the "compile incude" option, rather than specially exposing internals to set of exporters..)

[CodeBlanch]

I tried a couple of things. Ultimately pushed a change to fix the benchmark project by using an alias. It is nasty, but at least the nasty is localized to just benchmarks. Check it out, LMK. Might be worth just making it public 🤣

One neat thing I found doing this, you can link in an entire folder like this:

  <ItemGroup>
    <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\**\*.cs" LinkBase="Includes\" />
  </ItemGroup>

So we could possibly clean some of this up (somewhat) by organizing things into a couple of standard folders we link into exporters, instrumentation, etc?

[pellared]

Sorry that it took me so long, but I was 🤒

Ultimately pushed a change to fix the benchmark project by using an alias

Thank you 👍

Might be worth just making it public

I think it makes sense if we do not have any concrete argument against doing it (other than that it will increase the API). Do we have an example that making it public would make some harm?

you can link in an entire folder like this

I think it can be good. However, probably we do not want to include the whole Internal folder but rather some subfolder(s). Also, I think it could be done in a separate PR.

[cijothomas]

We discussed this in todays SIG meeting. Cijo/Alan agree to make this public, as it is implementing something from the spec.

[pellared]

Closing per #2350 (comment)

@CodeBlanch feel free to reopen and continue the discussion if you have any concerns