-
Notifications
You must be signed in to change notification settings - Fork 657
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
split zipkin exporter into json/proto packages #1699
split zipkin exporter into json/proto packages #1699
Conversation
IpInput = Union[str, int, None] | ||
|
||
|
||
class NodeEndpoint: |
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.
This class looks the same as the one in the JSON exporter opentelemetry.exporter.zipkin.node_endpoint.NodeEndpoint
, can we just reuse it? I see we are using the existing one in the file below (exporter/opentelemetry-exporter-zipkin-proto/tests/encoder/test_v2_protobuf.py)
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.
@aabmass thanks for catching this, i thought i'd removed it already
I think this is conflicting with the original motive and defeats the purpose of splitting up the packages to certain extent. There maybe no additional deps for now but we don't guarantee same holds in future. If I understand correctly taking dependency on json here would allow us reuse the some parts such as Lines 49 to 83 in f92431e
And I can see same functionality like compression in OTLP likely to be repeated. These are just few examples. Thoughts on this? |
I wouldn't worry about a bit of duplicated code especially if it is straightforward and simple enough. If someone ends up fixing a bug a few times and repeating themselves across multiple packages, I'm sure they'll refactor and take out the common code to make it re-usable. IMO let's not prematurely optimize for code-reuse. A bit of copying is fine :) |
Agree that the JSON package may add deps in future that would be useless for the proto package. I'm also inclined to separate them from the beginning but also fine if we document this in code and make it clear that if/when JSON package adds deps that proto does not need, proto should drop it as a dep. May be we can go with the approach suggested and create an issue to split later if this saves us time. |
I think proto is taking json as dependency for things like |
CHANGELOG.md
Outdated
@@ -68,6 +68,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
([#1695](https://github.com/open-telemetry/opentelemetry-python/pull/1695)) | |||
- Change Jaeger exporters to obtain service.name from span | |||
([#1703](https://github.com/open-telemetry/opentelemetry-python/pull/1703)) | |||
- Split Zipkin exporter into `opentelemetry-exporter-zipkin-json` and | |||
`opentelemetry-exporter-zipkin-proto` packages to reduce dependencies. The |
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.
Should we be more explicit in package naming opentelemetry-exporter-zipkin-proto-http
?
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.
Updated. I'll update the jaeger proto package in a separate PR to follow the same pattern opentelemetry-exporter-jaeger-proto-grpc
.
I was wondering if it makes sense to add a protocol to the end of the other packages as well ie. opentelemetry-exporter-zipkin-json
opentelemetry-exporter-jaeger-thrift
? And should the jaeger package be split into a UDP and HTTP package?
In my opinion the big dependencies users would want to to avoid are protos and grpc, in which case i would vote to leave the other packages as is.
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.
In my opinion the big dependencies users would want to to avoid are protos and grpc, in which case i would vote to leave the other packages as is.
Agree. I don't think there isn't much we gain from splitting them further.
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.
+1 on leaving packages as is.
@@ -96,7 +97,7 @@ | |||
class ZipkinExporter(SpanExporter): | |||
def __init__( | |||
self, | |||
protocol: Protocol, | |||
version: Protocol = Protocol.V2, |
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.
I think there is no default in specification. Should we make V2 json as default for this release?
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.
the old default was v2 protobuf, i think v2 makes sense here.
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.
I had sent a PR to make v2 default but it was rejected and spec was updated with this instead.
Addtionally, the following environment variables are reserved for future usage in Zipkin Exporter configuration:
OTEL_EXPORTER_ZIPKIN_PROTOCOL
This will be used to specify whether or not the exporter uses v1 or v2, json, thrift or protobuf. As of 1.0 of the specification, there is no specified default, or configuration via environment variables.
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.
Ah interesting. I guess we make a choice for the default used for the proto package since only v2 is implemented in that protocol now. I'm ok w/ using the same default for json
Description
Splitting the zipkin exporter package into
opentelemetry-exporter-zipkin-json
andopentelemetry-exporter-zipkin-proto
packages. Note that since there's no additional dependencies foropentelemetry-exporter-zipkin-json
, I used it as the base foropentelemetry-exporter-zipkin-proto
. I'm open to changing this if it's too confusing.Last part of #1604
Type of change
Please delete options that are not relevant.
Does This PR Require a Contrib Repo Change?
Checklist: