Make OtlpExporter class internal #1528
Conversation
| @@ -32,7 +32,7 @@ namespace OpenTelemetry.Exporter.OpenTelemetryProtocol | |||
| /// Exporter consuming <see cref="Activity"/> and exporting the data using | |||
| /// the OpenTelemetry protocol (OTLP). | |||
| /// </summary> | |||
| public class OtlpExporter : BaseExporter<Activity> | |||
| internal class OtlpExporter : BaseExporter<Activity> | |||
There was a problem hiding this comment.
the ctor() was already internal, so we wont break anyone right?
There was a problem hiding this comment.
By the class being public, the type is visible, so a user can type var exporter = new OtlpExporter() ... but with no way to construct it, is there any value in exposing it?
Ah, sorry, I misread your question... it does not appear to break anything.
There was a problem hiding this comment.
Agree.
I was thinking -- if a user want to write own Batching/SimpleExportprocessor, then they wont be able to do that, as they cannot construct the exporter. But its okay, as spec talks about only 2 ExportingProcessors. and #1504 will solve customization issues as well.
There was a problem hiding this comment.
Right, if #1504 is the pattern we want going forward, then I don't think we need to expose the classes. We could always re-expose the classes with a public constructor if we saw some need for that in the future.
Codecov Report
@@ Coverage Diff @@
## master #1528 +/- ##
==========================================
- Coverage 81.40% 81.39% -0.02%
==========================================
Files 233 233
Lines 6358 6358
==========================================
- Hits 5176 5175 -1
- Misses 1182 1183 +1
|
The constructor on all of our exporter classes is internal. I think making the class internal is the right thing to do. If so, I can expand this PR to make this change to zipkin, jaeger, etc.
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes