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

Migrate ZipkinExporter to BatchExportActivityProcessor #1103

Merged
merged 5 commits into from
Aug 20, 2020
Merged

Migrate ZipkinExporter to BatchExportActivityProcessor #1103

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a161b6
migrate zipkin exporter to BatchExportActivityProcessor
reyang Aug 20, 2020
8ef24c1
update changelog
reyang Aug 20, 2020
407b750
fix test failure
reyang Aug 20, 2020
365c936
Merge branch 'master' into reyang/zipkin
cijothomas Aug 20, 2020
47b3bc4
Merge branch 'master' into reyang/zipkin
reyang Aug 20, 2020
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
27 changes: 12 additions & 15 deletions src/OpenTelemetry.Exporter.Zipkin/ZipkinExporter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ namespace OpenTelemetry.Exporter.Zipkin
/// <summary>
/// Zipkin exporter.
/// </summary>
public class ZipkinExporter : ActivityExporter
public class ZipkinExporter : ActivityExporterSync
{
private readonly ZipkinExporterOptions options;
private readonly HttpClient httpClient;
Expand All @@ -57,33 +57,30 @@ public ZipkinExporter(ZipkinExporterOptions options, HttpClient client = null)
internal ZipkinEndpoint LocalEndpoint { get; }

/// <inheritdoc/>
public override async Task<ExportResult> ExportAsync(IEnumerable<Activity> batchActivity, CancellationToken cancellationToken)
public override ExportResultSync Export(in Batch<Activity> batch)
{
try
{
await this.SendBatchActivityAsync(batchActivity, cancellationToken).ConfigureAwait(false);
// take a snapshot of the batch
var activities = new List<Activity>();
Copy link
Member

Choose a reason for hiding this comment

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

A lot of work to avoid allocations just to allocate 🤣 What's the plan here?

Copy link
Member Author

@reyang reyang Aug 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The plan is to change the serializer to take the zero-alloc foreach (and potentially streaming JSON writer) instead of IEnumerator.
For now, the immediate goal is to migrate/clean up so we have a stable API for Beta-2, and do micro optimization in separate PRs.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good I figured you would have a plan for it.

foreach (var activity in batch)
{
activities.Add(activity);
}

this.SendBatchActivityAsync(activities, CancellationToken.None).RunSynchronously();

return ExportResult.Success;
return ExportResultSync.Success;
}
catch (Exception ex)
{
ZipkinExporterEventSource.Log.FailedExport(ex);

// TODO distinguish retryable exceptions
Copy link
Member

Choose a reason for hiding this comment

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

Is this still a relevant TODO?

Copy link
Member

Choose a reason for hiding this comment

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

dont think so. but not because of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it is still a TODO, the scheduler and flush (and potentially offline storage) part still needs to be improved.

Copy link
Member

Choose a reason for hiding this comment

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

Yes to that part.

I meant that there is no distinct return type for Failed vs FailedButRetryable. Spec was modified to have only 2 return types back to Processor- Success or Failure.

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#span-exporter

return ExportResult.FailedNotRetryable;
return ExportResultSync.Failure;
}
}

/// <inheritdoc/>
public override Task ShutdownAsync(CancellationToken cancellationToken)
{
#if NET452
return Task.FromResult(0);
#else
return Task.CompletedTask;
#endif
}

private static string ResolveHostAddress(string hostName, AddressFamily family)
{
string result = null;
Expand Down
2 changes: 1 addition & 1 deletion src/OpenTelemetry.Exporter.Zipkin/ZipkinExporterHelperExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public static TracerProviderBuilder AddZipkinExporter(this TracerProviderBuilder
var zipkinExporter = new ZipkinExporter(exporterOptions);

// TODO: Pick Simple vs Batching based on ZipkinExporterOptions
return builder.AddProcessor(new SimpleActivityProcessor(zipkinExporter));
return builder.AddProcessor(new BatchExportActivityProcessor(zipkinExporter));
}
}
}
9 changes: 4 additions & 5 deletions test/Benchmarks/Exporter/ZipkinExporterBenchmarks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
using BenchmarkDotNet.Attributes;
using OpenTelemetry.Exporter.Zipkin;
using OpenTelemetry.Tests;
using OpenTelemetry.Trace;

namespace Benchmarks.Exporter
{
Expand Down Expand Up @@ -60,21 +61,19 @@ public void GlobalCleanup()
}

[Benchmark]
public async Task ZipkinExporter_ExportAsync()
public void ZipkinExporter_Export()
{
var zipkinExporter = new ZipkinExporter(
new ZipkinExporterOptions
{
Endpoint = new Uri($"http://{this.serverHost}:{this.serverPort}"),
});
var processor = new SimpleExportActivityProcessor(zipkinExporter);

var activities = new List<Activity>(this.NumberOfActivities);
for (int i = 0; i < this.NumberOfActivities; i++)
{
activities.Add(this.testActivity);
processor.OnEnd(this.testActivity);
}

await zipkinExporter.ExportAsync(activities, CancellationToken.None).ConfigureAwait(false);
}

private Activity CreateTestActivity()
Expand Down
10 changes: 4 additions & 6 deletions test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,8 @@ public void ZipkinExporter_BadArgs()
[Theory]
[InlineData(true)]
[InlineData(false)]
public async Task ZipkinExporterIntegrationTest(bool useShortTraceIds)
public void ZipkinExporterIntegrationTest(bool useShortTraceIds)
{
var batchActivity = new List<Activity> { CreateTestActivity() };

Guid requestId = Guid.NewGuid();

ZipkinExporter exporter = new ZipkinExporter(
Expand All @@ -107,11 +105,11 @@ public async Task ZipkinExporterIntegrationTest(bool useShortTraceIds)
UseShortTraceIds = useShortTraceIds,
});

await exporter.ExportAsync(batchActivity, CancellationToken.None).ConfigureAwait(false);
var activity = CreateTestActivity();
var processor = new SimpleExportActivityProcessor(exporter);

await exporter.ShutdownAsync(CancellationToken.None).ConfigureAwait(false);
processor.OnEnd(activity);

var activity = batchActivity[0];
var context = activity.Context;

var timestamp = activity.StartTimeUtc.ToEpochMicroseconds();
Expand Down