-
Notifications
You must be signed in to change notification settings - Fork 773
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.
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
Changes from 3 commits
0a161b6
8ef24c1
407b750
365c936
47b3bc4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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>(); | ||
foreach (var activity in batch) | ||
{ | ||
activities.Add(activity); | ||
} | ||
|
||
this.SendBatchActivityAsync(activities, CancellationToken.None).GetAwaiter().GetResult(); | ||
|
||
return ExportResult.Success; | ||
return ExportResultSync.Success; | ||
} | ||
catch (Exception ex) | ||
{ | ||
ZipkinExporterEventSource.Log.FailedExport(ex); | ||
|
||
// TODO distinguish retryable exceptions | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this still a relevant TODO? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dont think so. but not because of this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
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; | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.