Added Exporter type to Zipkin exporter options #1504
Conversation
src/OpenTelemetry.Exporter.Zipkin/ZipkinExporterHelperExtensions.cs
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,24 @@ | |||
| using System; | |||
| using System.Collections.Generic; | |||
| using System.Linq; | |||
There was a problem hiding this comment.
Seems most of the using statements are not needed?
src/OpenTelemetry.Exporter.Zipkin/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #1504 +/- ##
==========================================
- Coverage 81.39% 81.27% -0.12%
==========================================
Files 232 233 +1
Lines 6357 6370 +13
==========================================
+ Hits 5174 5177 +3
- Misses 1183 1193 +10
|
…com/utpilla/opentelemetry-dotnet into utpilla/Extend-ZipkinExporteOptions
| namespace OpenTelemetry | ||
| { | ||
|
|
||
| internal static class BatchExportProcessorDefaultOptions |
There was a problem hiding this comment.
Do we need this class, or these consts can be part of the BatchExportProcessor? (e.g. BatchExportProcessor.DefaultMaxQueueSize)?
There was a problem hiding this comment.
I would move them onto BatchExportProcessorOptions and add "Default" prefix. Primary benefit being discoverability, they will show up on intellisense when user interacts with options instance.
There was a problem hiding this comment.
I have moved them to BatchExportProcessor as these are the default values to be used for this for BatchExportProcessor ctor parameters. BatchExportProcessorOptions allows the users to change this value but it has the default values for each of the properties mentioned in the comment. This way it would show up on the property description on Intellisense.
| { | ||
| public class BatchExportProcessorOptions | ||
| { | ||
| public int MaxQueueSize { get; set; } = BatchExportProcessorDefaultOptions.MaxQueueSize; |
There was a problem hiding this comment.
Do we need units on the two size props? Or maybe just some renaming like MaxQueueLength/MaxBatchCount?
There was a problem hiding this comment.
I'll rename it to MaxQueueLength. For, MaxBatchSize vs MaxBatchCount, I feel "size" goes well with batch as opposed to "count". I have no strong preference though.
There was a problem hiding this comment.
@CodeBlanch @utpilla Just a question for my own edification... what has been our precedent for naming things same/similar to names suggested in the spec? Have we been somewhat flexible in this regard?
I'm just curious, no reason to hold up this PR.
There was a problem hiding this comment.
I have renamed the BatchExportProcessorOptions properties to match the SDK spec since the file is now moved to OpenTelemetry and is not under OpenTelemetry.Exporter.Zipkin.
There was a problem hiding this comment.
My default choice is - pick what spec says, so it'll be consistent across all other implementations. If the spec name doesn't make sense, send a proposal to spec repo - its usually fast to get approvals if the proposed name is better at clarifying things.
Co-authored-by: Cijo Thomas <[email protected]>
Co-authored-by: Cijo Thomas <[email protected]>
…com/utpilla/opentelemetry-dotnet into utpilla/Extend-ZipkinExporteOptions
Changes
-Added ExporterType to ZipkinExporter options
-Added BatchExportProcessor ctor parameters as ZipkinExporter options
Addressing issue #1493