-
Notifications
You must be signed in to change notification settings - Fork 889
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
Clarify placement of exporter implementation #277
Clarify placement of exporter implementation #277
Conversation
@@ -16,7 +16,7 @@ _Note to Language Library Authors:_ OpenTelemetry specification, API and SDK imp | |||
|
|||
3. The developers of the final application normally decide how to configure OpenTelemetry SDK and what extensions to use. They should be also free to choose to not use any OpenTelemetry implementation at all, even though the application and/or its libraries are already instrumented. The rationale is that third-party libraries and frameworks which are instrumented with OpenTelemetry must still be fully usable in the applications which do not want to use OpenTelemetry (so this removes the need for framework developers to have "instrumented" and "non-instrumented" versions of their framework). | |||
|
|||
4. Language library implementation must be clearly separated into wire protocol-independent parts that implement common logic (e.g. batching, tag enrichment by process information, etc.) and protocol-dependent telemetry exporters. Telemetry exporters must contain minimal functionality, thus enabling vendors to easily add support for their specific protocol to the language library. | |||
4. Language library implementation must be clearly separated into wire protocol-independent parts that implement common logic (e.g. batching, tag enrichment by process information, etc.) and protocol-dependent telemetry exporters. Telemetry exporters must contain minimal functionality, thus enabling vendors to easily add support for their specific protocol. |
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.
To my reading, this still implies that support would be added to the open telemetry projects, not hosted by the vendors themselves. I think it should be made very clear that open telemetry will not be hosting vendor-specific exporters, but that vendors will need to provide those themselves, hosted outside of the OT project.
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.
So do Jaeger and Prometheus not count as vendors?
Also, Python currently has a MS Azure exporter.
EDIT: Maybe we should distinguish between repositories and packages here, e.g. while the Azure exporter in Python is in the same repository as the core OTel library, it has to be installed explicitly & separately.
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.
Probably off topic a little bit, besides exporters there are also propagators - currently have Zipkin specific correlation (a.k.a. B3 headers) in OT Python - and it is part of the core SDK (the same repo, and the same package) https://github.com/open-telemetry/opentelemetry-python/blob/master/opentelemetry-sdk/src/opentelemetry/sdk/context/propagation/b3_format.py.
@tigrannajaryan please make it very clear that we do not allow any vendor specific code in the repos. |
IIRC we were supposed to host Jaeger/Zipkin/Prometheus exporters, both as reference and also because of their open source nature (as compared to, say, the Azure one, which will have to be moved out eventually). That means we should prepare for putting the aforementioned exporters in their own repo? |
There was unnecessarily suggestive wording that implied that vendor exporters should be part of language libraries. Added a clarification regarding vendor-specific exporters, OpenTelemetry protocol exporter and standard output exporter.
b2d8f7a
to
3003aa7
Compare
I added further clarification as discussed in spec SIG meeting today. See requirement 5. |
|
||
5. Language library implementation should include an exporter for OpenTelemetry Protocol (when the protocol is specified and approved) and may include an exporter that writes to standard output (to use for debugging and testing). Vendor-specific exporters (exporters that implement vendor protocols) should not be included in language libraries and should be placed elsewhere (the exact approach for storing and maintaining vendor-specific exporters will be defined in the future). |
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.
Vendor-specific exporters (exporters that implement vendor protocols) should not be included in language libraries and should be placed elsewhere (the exact approach for storing and maintaining vendor-specific exporters will be defined in the future).
Right now we keep some vendor-specific exporters in the OT python repo, but publish separate distribution packages for each. So e.g. to export traces to azure monitor, a user would have to install opentelemetry-ext-azure-monitor.
It's not clear from the text, but from the SIG call this morning it sounds like these exporters should also be in separate repos from the main project?
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.
IMO, vendor-specific exporters do not belong neither in the Otel repo, nor in the distrubution.
The reason for not including in the repo is to avoid additional maintanance burden for core maintainers.
The reason for not including in the distribution is to avoid bloating packaging and runtime dependency applications must use.
Now, if a particular language SIG decides they want to have batteries-included SDK I do not see a big harm if they provide 2 packages: a bare "sdk" which does not include vendor-specific exporters and an "sdk-full" which includes them. Just make sure to have enough transparency and avoid vendor-influenced decision when doing so.
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.
That sounds reasonable to me, but this is likely to mean more maintenance work for us, not less. Even still, it might be worth making this change to avoid privileging the exporters we maintain over others.
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.
@c24t we are having a similar discussion on the ruby-side in regards to auto-instrumentation, and it will be part of our discussion for exporters. The thinking being it may be advantageous in the short-term to have them in the same repo, especially while the api/sdk libs are still forming. The privileging aspect is something I hadn't considered.
What's the motivation for putting an extensible exporter class in the SDK instead of an interface in the API? As I understand it, we've got three personas to consider:
Library devs shouldn't use exporters, so from their perspective it makes sense to leave them out of the API. But then vendors are forced to depend on the SDK, even if they're just using it to type-check their exporter implementation. I know this has been covered before, but I don't see a good source in this repo. Please let me know if there's a discussion in a comment or doc somewhere. Motivated by a question on python metrics exporters. |
I agree. There is no need to make exporters and exporter API a dependency for library devs.
They only need to depend on Exporter interface (https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/sdk-tracing.md#span-exporter). The spec does not currently define whether Exporter interface can be a separate package so that vendor-specific exporter have less dependencies. Probably it should, we can clarify this in a separate PR. |
|
||
5. Language library implementation should include an exporter for OpenTelemetry Protocol (when the protocol is specified and approved) and may include an exporter that writes to standard output (to use for debugging and testing). Vendor-specific exporters (exporters that implement vendor protocols) should not be included in language libraries and should be placed elsewhere (the exact approach for storing and maintaining vendor-specific exporters will be defined in the future). |
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.
Can we please include Zipkin and/or Jaeger for tracing and Prometheus for metrics as a privileged exporters which are recommended to be implemented for all languages.
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.
@SergeyKanzhelev I am happy to make the change in this PR if the Spec SIG / TC makes the decision.
It looks like at least one language library author (@c24t) believes including Zipkin, Jaeger, Prometheus in the SDK is easier from maintenance perspective than keeping it to a separate repo. If we have other voices supporting this position I think we should go ahead with that.
One more related data point: for OpenTelemetry Collector we decided that OpenCensus, Zipkin, Jaeger, Prometheus protocol support will be part of Collector "core" while the rest will be moved to a "contrib" repo. This is because we believed the Collector artifact that we produce needs to be usable in production as-is without requiring end users to build their own (which obviously requires some real-world protocols to be supported). The requirements for SDK are a bit different (end users of SDK are building their own apps anyway) but if we aim for uniformity then we can decide to support the same protocols in core SDKs.
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.
If you can include the note about exporters that we can recommend as a demo exporters - this PR will close this issue #6
@SergeyKanzhelev sure, I can add a note. Do you want to make a call as a TC member and post the list of recommended exporters here or in the linked issue and I can make it part of this PR? |
@tigrannajaryan would you please make this change as a separate PR. I'm merging this one |
Based on previous discussion [1] this PR adds a recommendation to include exporters for several open-source protocols in language libraries. [1] - open-telemetry#277 (comment)
Based on previous discussion [1] this PR adds a recommendation to include exporters for several open-source protocols in language libraries. [1] - open-telemetry#277 (comment)
Based on previous discussion [1] this PR adds a recommendation to include exporters for several open-source protocols in language libraries. Addresses issue open-telemetry#6 [1] - open-telemetry#277 (comment)
Added a separate PR to describe recommended exporters: #284 |
* Add list of exporters that are recommended for SDK Based on previous discussion [1] this PR adds a recommendation to include exporters for several open-source protocols in language libraries. Addresses issue #6 [1] - #277 (comment) * PR fixes
There was unnecessarily suggestive wording that implied that vendor exporters should be part of language libraries. Added a clarification regarding vendor-specific exporters, OpenTelemetry protocol exporter and standard output exporter.
* Add list of exporters that are recommended for SDK Based on previous discussion [1] this PR adds a recommendation to include exporters for several open-source protocols in language libraries. Addresses issue open-telemetry#6 [1] - open-telemetry#277 (comment) * PR fixes
There was unnecessarily suggestive wording that implied that vendor exporters should be part of language libraries. Added a clarification regarding vendor-specific exporters, OpenTelemetry protocol exporter and standard output exporter.
There was unnecessarily suggestive wording that implied that vendor
exporters should be part of language libraries.
Added a clarification regarding vendor-specific exporters,
OpenTelemetry protocol exporter and standard output exporter.