split zipkin exporter into json/proto packages

[codeboten]

Description

Splitting the zipkin exporter package into opentelemetry-exporter-zipkin-json and opentelemetry-exporter-zipkin-proto packages. Note that since there's no additional dependencies for opentelemetry-exporter-zipkin-json, I used it as the base for opentelemetry-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.

• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Does This PR Require a Contrib Repo Change?

• No.

Checklist:

• Followed the style guidelines of this project
• Changelogs have been updated
• Unit tests have been added
• Documentation has been updated

[aabmass]

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)

[codeboten]

@aabmass thanks for catching this, i thought i'd removed it already

[srikanthccv]

Note that since there's no additional dependencies for opentelemetry-exporter-zipkin-json, I used it as the base for opentelemetry-exporter-zipkin-proto.

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 Encoder etc... I have some broad question here that applies to all exporters. How do we not repeat the certain abstractions or same functionality across the split packages? Take jaeger for instance, we have same Translator abstraction in both Protobuf and Thrift packages.

opentelemetry-python/exporter/opentelemetry-exporter-jaeger-thrift/src/opentelemetry/exporter/jaeger/thrift/translate/__init__.py

Lines 49 to 83 in f92431e

	class Translator(abc.ABC):
	def __init__(self, max_tag_value_length: Optional[int] = None):
	self._max_tag_value_length = max_tag_value_length
	@abc.abstractmethod
	def _translate_span(self, span):
	"""Translates span to jaeger format.
	Args:
	span: span to translate
	"""
	@abc.abstractmethod
	def _extract_tags(self, span):
	"""Extracts tags from span and returns list of jaeger Tags.
	Args:
	span: span to extract tags
	"""
	@abc.abstractmethod
	def _extract_refs(self, span):
	"""Extracts references from span and returns list of jaeger SpanRefs.
	Args:
	span: span to extract references
	"""
	@abc.abstractmethod
	def _extract_logs(self, span):
	"""Extracts logs from span and returns list of jaeger Logs.
	Args:
	span: span to extract logs
	"""

And I can see same functionality like compression in OTLP likely to be repeated. These are just few examples. Thoughts on this?

[owais]

I have some broad question here that applies to all exporters. How do we not repeat the certain abstractions or same functionality across the split packages? Take jaeger for instance, we have same Translator abstraction in both Protobuf and Thrift packages.

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 :)

[owais]

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.

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.

[srikanthccv]

I think proto is taking json as dependency for things like encoder package and node_endpoint module etc... And if it was not to take dependency it would have to copy all of it which I find bothering me. I was just wondering if we could have something like otel-exporter-{}-common. That can be addressed in another issue as it not something significant to worry for release.

[srikanthccv]

Should we be more explicit in package naming opentelemetry-exporter-zipkin-proto-http?

[codeboten]

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.

[srikanthccv]

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.

[lzchen]

+1 on leaving packages as is.

[srikanthccv]

I think there is no default in specification. Should we make V2 json as default for this release?

[codeboten]

the old default was v2 protobuf, i think v2 makes sense here.

[srikanthccv]

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.

[codeboten]

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