Migrate ZipkinExporter to BatchExportActivityProcessor #1103
Conversation
|
|
||
| return ExportResult.Success; | ||
| return ExportResultSync.Success; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| ZipkinExporterEventSource.Log.FailedExport(ex); | ||
|
|
||
| // TODO distinguish retryable exceptions |
There was a problem hiding this comment.
Is this still a relevant TODO?
There was a problem hiding this comment.
dont think so. but not because of this PR.
There was a problem hiding this comment.
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.
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.
|
@reyang , are we going to move all exporters to the new batch implementation? |
Yes, the old one has unbounded behavior such like shooting random Tasks, we'll remove the old one once we've moved everything over. |
Ok! I can help you with the other exporters :) |
Codecov Report
@@ Coverage Diff @@
## master #1103 +/- ##
==========================================
+ Coverage 74.71% 76.05% +1.34%
==========================================
Files 223 223
Lines 6379 6381 +2
==========================================
+ Hits 4766 4853 +87
+ Misses 1613 1528 -85
|
reyang
changed the title
reyang
changed the title
| { | ||
| try | ||
| { | ||
| await this.SendBatchActivityAsync(batchActivity, cancellationToken).ConfigureAwait(false); | ||
| // take a snapshot of the batch | ||
| var activities = new List<Activity>(); |
There was a problem hiding this comment.
A lot of work to avoid allocations just to allocate 🤣 What's the plan here?
There was a problem hiding this comment.
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.
Sounds good I figured you would have a plan for it.
Related to #1078.
Changes
Please provide a brief description of the changes here. Update the
CHANGELOG.mdfor non-trivial changes.For significant contributions please make sure you have completed the following items: