-
Notifications
You must be signed in to change notification settings - Fork 894
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
Changes from 1 commit
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 |
---|---|---|
|
@@ -16,8 +16,9 @@ _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. | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe 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 commentThe 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 commentThe 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. 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. 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 commentThe 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. |
||
|
||
# Language Library Generic Design | ||
|
||
|
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.