Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make BatchExportActivityProcessorOptions internal #2350

Closed
wants to merge 7 commits into from
Closed

Make BatchExportActivityProcessorOptions internal #2350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5eff2e0
Make BatchExportActivityProcessorOptions internal
pellared Sep 14, 2021
0bc8c90
Remove unnecessary Compile Include
pellared Sep 14, 2021
7b0b6a8
Merge branch 'main' into make-BatchExportActivityProcessorOptions-int…
cijothomas Sep 17, 2021
35cc53f
Merge branch 'main' into make-BatchExportActivityProcessorOptions-int…
cijothomas Sep 17, 2021
0e16a33
Merge branch 'main' into make-BatchExportActivityProcessorOptions-int…
pellared Sep 21, 2021
8b25442
Update CHANGELOG.md
pellared Sep 21, 2021
1c3fa18
Use alias to fix InternalsVisibleTo issues in benchmark project.
CodeBlanch Sep 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/OpenTelemetry.Exporter.Jaeger/OpenTelemetry.Exporter.Jaeger.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\PeerServiceResolver.cs" Link="Includes\PeerServiceResolver.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\ResourceSemanticConventions.cs" Link="Includes\ResourceSemanticConventions.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\ServiceProviderExtensions.cs" Link="Includes\ServiceProviderExtensions.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\BatchExportActivityProcessorOptions.cs" Link="Includes\BatchExportActivityProcessorOptions.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\OpenTelemetrySdkEventSource.cs" Link="Includes\OpenTelemetrySdkEventSource.cs" />
</ItemGroup>

<ItemGroup>
Expand Down
2 changes: 2 additions & 0 deletions src/OpenTelemetry.Exporter.Zipkin/OpenTelemetry.Exporter.Zipkin.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\PeerServiceResolver.cs" Link="Includes\PeerServiceResolver.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\ResourceSemanticConventions.cs" Link="Includes\ResourceSemanticConventions.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\ServiceProviderExtensions.cs" Link="Includes\ServiceProviderExtensions.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\BatchExportActivityProcessorOptions.cs" Link="Includes\BatchExportActivityProcessorOptions.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry\Internal\OpenTelemetrySdkEventSource.cs" Link="Includes\OpenTelemetrySdkEventSource.cs" />
</ItemGroup>

<ItemGroup>
Expand Down
2 changes: 0 additions & 2 deletions src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +0,0 @@
OpenTelemetry.Trace.BatchExportActivityProcessorOptions
OpenTelemetry.Trace.BatchExportActivityProcessorOptions.BatchExportActivityProcessorOptions() -> void
2 changes: 0 additions & 2 deletions src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +0,0 @@
OpenTelemetry.Trace.BatchExportActivityProcessorOptions
OpenTelemetry.Trace.BatchExportActivityProcessorOptions.BatchExportActivityProcessorOptions() -> void
2 changes: 2 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Unreleased

* `BatchExportActivityProcessorOptions` is made internal. ([#2350](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2350))

* Changed `CompositeProcessor<T>.OnForceFlush` to meet with the spec
requirement. Now the SDK will invoke `ForceFlush` on all registered
processors, even if there is a timeout.
Expand Down
2 changes: 1 addition & 1 deletion ...ce/BatchExportActivityProcessorOptions.cs → ...al/BatchExportActivityProcessorOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

namespace OpenTelemetry.Trace
{
public class BatchExportActivityProcessorOptions : BatchExportProcessorOptions<Activity>
internal sealed class BatchExportActivityProcessorOptions : BatchExportProcessorOptions<Activity>
Copy link
Member

@cijothomas cijothomas Sep 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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..)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
internal const string MaxQueueSizeEnvVarKey = "OTEL_BSP_MAX_QUEUE_SIZE";

Expand Down
4 changes: 2 additions & 2 deletions test/Benchmarks/Benchmarks.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@

<ItemGroup>
<ProjectReference Include="$(RepoRoot)\src\OpenTelemetry\OpenTelemetry.csproj" />
<ProjectReference Include="$(RepoRoot)\src\OpenTelemetry.Exporter.Jaeger\OpenTelemetry.Exporter.Jaeger.csproj" />
<ProjectReference Include="$(RepoRoot)\src\OpenTelemetry.Exporter.Zipkin\OpenTelemetry.Exporter.Zipkin.csproj" />
<ProjectReference Aliases="Jaeger" Include="$(RepoRoot)\src\OpenTelemetry.Exporter.Jaeger\OpenTelemetry.Exporter.Jaeger.csproj" />
<ProjectReference Aliases="Zipkin" Include="$(RepoRoot)\src\OpenTelemetry.Exporter.Zipkin\OpenTelemetry.Exporter.Zipkin.csproj" />
<ProjectReference Include="$(RepoRoot)\src\OpenTelemetry.Instrumentation.AspNetCore\OpenTelemetry.Instrumentation.AspNetCore.csproj" />
<ProjectReference Include="$(RepoRoot)\src\OpenTelemetry.Instrumentation.Http\OpenTelemetry.Instrumentation.Http.csproj" />
<ProjectReference Include="$(RepoRoot)\src\OpenTelemetry.Exporter.OpenTelemetryProtocol\OpenTelemetry.Exporter.OpenTelemetryProtocol.csproj" />
Expand Down
8 changes: 5 additions & 3 deletions test/Benchmarks/Exporter/JaegerExporterBenchmarks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@
// limitations under the License.
// </copyright>

extern alias Jaeger;

using System.Diagnostics;
using BenchmarkDotNet.Attributes;
using Benchmarks.Helper;
using Jaeger::OpenTelemetry.Exporter;
using Jaeger::Thrift.Transport;
using OpenTelemetry;
using OpenTelemetry.Exporter;
using OpenTelemetry.Internal;
using Thrift.Transport;

namespace Benchmarks.Exporter
{
Expand Down Expand Up @@ -50,7 +52,7 @@ public void JaegerExporter_Batching()
new JaegerExporterOptions(),
new BlackHoleTransport())
{
Process = new OpenTelemetry.Exporter.Jaeger.Implementation.Process("TestService"),
Process = new Jaeger::OpenTelemetry.Exporter.Jaeger.Implementation.Process("TestService"),
};

for (int i = 0; i < this.NumberOfBatches; i++)
Expand Down
4 changes: 3 additions & 1 deletion test/Benchmarks/Exporter/ZipkinExporterBenchmarks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,17 @@
// limitations under the License.
// </copyright>

extern alias Zipkin;

using System;
using System.Diagnostics;
using System.IO;
using BenchmarkDotNet.Attributes;
using Benchmarks.Helper;
using OpenTelemetry;
using OpenTelemetry.Exporter;
using OpenTelemetry.Internal;
using OpenTelemetry.Tests;
using Zipkin::OpenTelemetry.Exporter;

namespace Benchmarks.Exporter
{
Expand Down