Add protobuf via gRPC exporting support for Jaeger #1471
Conversation
srikanthccv
requested review from
a team,
toumorokoshi and
aabmass
and removed request for
a team
exporter/opentelemetry-exporter-jaeger/examples/jaeger_exporter_example.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/__init__.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/__init__.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/__init__.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/__init__.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/translate/protobuf.py
Show resolved
Hide resolved
exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/translate/protobuf.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/util.py
Show resolved
Hide resolved
exporter/opentelemetry-exporter-jaeger/tests/test_jarget_exporter_protobuf.py
Outdated
Show resolved
Hide resolved
| @@ -26,7 +26,6 @@ envlist = | |||
|
|
|||
| ; opentelemetry-exporter-jaeger | |||
| py3{5,6,7,8,9}-test-exporter-jaeger | |||
| pypy3-test-exporter-jaeger | |||
There was a problem hiding this comment.
Why is this being removed?
There was a problem hiding this comment.
https://github.com/open-telemetry/opentelemetry-python/blob/master/tox.ini#L31-L37
I think pypy3 doesn't work with exporters which have protobuf via gRPC. It is excluded from otlp and opencensus. It was failing for jaeger as well, that's why I had removed this.
| @@ -39,6 +39,8 @@ package_dir= | |||
| =src | |||
| packages=find_namespace: | |||
| install_requires = | |||
| grpcio >= 1.0.0, < 2.0.0 | |||
There was a problem hiding this comment.
since this isn't required for all exporter configurations, should this be an extras_require?
I personally don't care about bloating package dependencies and including unneeded ones, but I've found that to be a prickly subject for others.
There was a problem hiding this comment.
I think this is needed as we have import for from grpc and typehints in certain places.
from grpc import (
ChannelCredentials,
insecure_channel,
secure_channel,
ssl_channel_credentials,
)| from opentelemetry.exporter.jaeger.gen.agent import Agent as agent | ||
| from opentelemetry.exporter.jaeger.gen.jaeger import Collector as jaeger | ||
|
|
||
| UDP_PACKET_MAX_LENGTH = 65000 |
There was a problem hiding this comment.
are you suggesting this is an issue? I personally kind of like having this defined. Learned something new about UDP :)
| buff = self.buffer.getvalue() | ||
| if len(buff) > self.max_packet_size: | ||
| logger.warning( | ||
| "Data exceeds the max UDP packet size; size %r, max %r", |
There was a problem hiding this comment.
since this is configurable, this error message won't always be accurate. Would it be better to reference max_packet_size?
There was a problem hiding this comment.
Isn't it already there?
logger.warning(
"Data exceeds the max UDP packet size; size %r, max %r",
len(buff),
self.max_packet_size,
)
Are you saying something different?
| ) | ||
| return | ||
|
|
||
| with socket.socket(socket.AF_INET, socket.SOCK_DGRAM) as udp_socket: |
There was a problem hiding this comment.
would you not want to re-use an existing socket here?
exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/translate/thrift.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/__init__.py
Outdated
Show resolved
Hide resolved
| jaeger_spans = protobuf._to_jaeger(spans, self.service_name) | ||
| batch = model_pb2.Batch(spans=jaeger_spans) | ||
| request = PostSpansRequest(batch=batch) |
There was a problem hiding this comment.
I think this if is justified, IIUC the point is that values from lines 225-227 aren't used if the second if isn't true. I see @ocelotl's point as well: it seems the above three can be moved to the below if.
We can't get rid of this if (I don't believe we can accept default protobuf values), but we can reduce unneeded work if it's not true.
Description
This change adds ability to export traces to Jaeger with protobuf via gRPC.
Fixes #1437
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Contrib Repo Change?
Checklist: