Separate protobuf encoding into a separate package

[jack-burridge-cfh]

Description

This separates Protobuf encoding into a separate package opentelemetry-exporter-otlp-proto-common which is then used by:

• opentelemetry-exporter-otlp-proto-grpc
• opentelemetry-exporter-otlp-proto-http

Fixes #3160

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Tests have been moved from opentelemetry-exporter-otlp-proto-grpc and opentelemetry-exporter-otlp-proto-http to the new package

Checklist:

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jack-burridge-cfh (48cfab5, 9e885f8, 3fae967, 1e7f6b1)

[jack-burridge-cfh]

I've also noticed that all log packaged are protected, as in start with _log should this also happen in the common package?

[jack-burridge-cfh]

@lzchen @srikanthccv I'm not entirely sure on the namings, at the moment I've added the encoders as:

• opentelemetry.exporter.otlp.proto.common.log_encoder.encode_logs
• opentelemetry.exporter.otlp.proto.common.metrics_encoder.encode_metrics
• opentelemetry.exporter.otlp.proto.common.trace_encoder.encode_spans

But I am now thinking:

• opentelemetry.exporter.otlp.proto.common._log_encoder.encode
• opentelemetry.exporter.otlp.proto.common.metric_encoder.encode
• opentelemetry.exporter.otlp.proto.common.trace_encoder.encode

[srikanthccv]

@jack-burridge-cfh you can resolve the outdated conversation if you believe the comment is addressed. I will take a look at the PR again.

[jack-burridge-cfh]

@jack-burridge-cfh you can resolve the outdated conversation if you believe the comment is addressed. I will take a look at the PR again.

Done

[jack-burridge-cfh]

@srikanthccv The issues with the checks seem to be git-based ones which I can't reproduce locally, so I don't know how to fix those

[lzchen]

@jack-burridge-cfh
Sorry about the inconvenience. Builds seem to be failing (not related to your PR). We have a tracking issue here and will work to resolve it asap. In the meantime, we will be reviewing your PR :)

[jack-burridge-cfh]

I ran markdown-link-check locally and can't reproduce the error so looks like the server was just down when it was doing the check

[srikanthccv]

Overall LGTM. I am thinking about the import paths. It seems like one would end up using aliases if they want to use encoders.

opentelemetry.exporter.otlp.proto.common.metrics_encoder import encode as metrics_encode
opentelemetry.exporter.otlp.proto.common.trace_encoder import encode as trace_encode

[jack-burridge-cfh]

Overall LGTM. I am thinking about the import paths. It seems like one would end up using aliases if they want to use encoders.

opentelemetry.exporter.otlp.proto.common.metrics_encoder import encode as metrics_encode
opentelemetry.exporter.otlp.proto.common.trace_encoder import encode as trace_encode

The decision here was a conscious one, basically most uses will not involve multiple types of encoders, and encourages you to keep them separate. I honestly cannot think of a reason you would, it's more likely an edge case. Like they are used here the encoders are likely to be used to write different types of exporters (Kafka etc) and these exporters are likely to be in different files.

[srikanthccv]

We have been explicit and verbose in the project. You can see it in many places in the project, including the exporters (OTLPSpanExporter, OTLPMetricExporter). And let's be explicit here as well.

[jack-burridge-cfh]

We have been explicit and verbose in the project. You can see it in many places in the project, including the exporters (OTLPSpanExporter, OTLPMetricExporter). And let's be explicit here as well.

Fair point. Done 😄

[srikanthccv]

LGTM from my side, I will let other approvers take a look once and then merge if they don't have any objections. Thank you for your continuous effort to for addressing the comments and get this merged.

[srikanthccv]

Solid refactoring! Thank you.

[jack-burridge-cfh]

Solid refactoring! Thank you.

It's why I use PyCharm 😄 It may be cumbersome but the refactoring is fantastic

[jack-burridge-cfh]

@srikanthccv Any reason this cant be merged? or are we waiting for more people to look at it because of the scope of the change?