Add possibility to split oversized udp batches#1500
Merged
lzchen merged 3 commits intoopen-telemetry:mainfrom Mar 8, 2021
Merged
Add possibility to split oversized udp batches#1500lzchen merged 3 commits intoopen-telemetry:mainfrom
lzchen merged 3 commits intoopen-telemetry:mainfrom
Conversation
janLo
requested review from
a team,
aabmass and
ocelotl
and removed request for
a team
|
|
codeboten
suggested changes
Jan 6, 2021
Contributor
codeboten
left a comment
codeboten
left a comment
There was a problem hiding this comment.
Thanks for the PR, it looks great. Just a couple of changes requested, please also add an entry to the changelog.
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
Contributor
|
hey @janLo any chance you can take a look at the comments on this PR so we can get it merged? |
Contributor
Author
|
Yes, sorry, I'm just very busy at the moment. It's on my list and I plan to get it finished ASAP. |
Contributor
No worries, thanks! |
Contributor
|
@janLo |
Contributor
Author
|
I hope to get to it next week. |
janLo
force-pushed
the
jaeger-split-batches
branch
2 times, most recently
from
7e0e58d to
1e654bd
Compare
janLo
added a commit
to janLo/opentelemetry-specification
that referenced
this pull request
Feb 26, 2021
This adds OTEL_EXPORTER_JAEGER_AGENT_SPLIT_OVERSIZED_BATCHES to the Jaeger exporter as porposed in open-telemetry/opentelemetry-python#1500
janLo
force-pushed
the
jaeger-split-batches
branch
6 times, most recently
from
267544e to
99abed7
Compare
Contributor
Author
|
Sorry for the noise, I haven't had a working python env at hand today. I fixed the mentioned issues and rebased everything on main. I also added a changelog entry and made a PR in the mentioned documentation repo: open-telemetry/opentelemetry-specification#1475 |
bogdandrutu
reviewed
Feb 26, 2021
exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/__init__.py
Show resolved
Hide resolved
janLo
force-pushed
the
jaeger-split-batches
branch
from
99abed7 to
3efc085
Compare
seemk
reviewed
Mar 3, 2021
exporter/opentelemetry-exporter-jaeger/src/opentelemetry/exporter/jaeger/send/thrift.py
Outdated
Show resolved
Hide resolved
janLo
force-pushed
the
jaeger-split-batches
branch
from
3efc085 to
b9bca15
Compare
If we use the BatchExportSpanProcessor combined with the JaegerSpanExporter and use instrumentations that add a lot of metadata to the spans like sqlalchemy, then we run occationally into the "Data exceeds the max UDP packet size" warning causing dropped spans and incomplete data. The option to reduce the general batch-size to a very small number (in my case >30) may cause a performance issue as the worker thread of the batch exporter gets very busy. Instead this change allows the user to ask the exporter to split oversized batches when they get detected and send the splits separately instead of dropping them. Depending on the usecase this is a better option than reducing the batch-size to a very small value because every now and then they contain a couple of large spans.
janLo
force-pushed
the
jaeger-split-batches
branch
from
b9bca15 to
7529752
Compare
Contributor
Author
|
Can I somehow rerun teh failed check? I don't think it has anything to do with my changes. |
seemk
approved these changes
Mar 4, 2021
codeboten
approved these changes
Mar 8, 2021
lzchen
approved these changes
Mar 8, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
If we use the BatchExportSpanProcessor combined with the
JaegerSpanExporter and use instrumentations that add a lot of metadata
to the spans like sqlalchemy, then we run occationally into the "Data
exceeds the max UDP packet size" warning causing dropped spans and
incomplete data.
The option to reduce the general batch-size to a very small number (in
my case >30) may cause a performance issue as the worker thread of the
batch exporter gets very busy. Instead this change allows the user to
ask the exporter to split oversized batches when they get detected and
send the splits separately instead of dropping them. Depending on the
usecase this is a better option than reducing the batch-size to a very
small value because every now and then they contain a couple of large
spans.
Type of change
How Has This Been Tested?
Does This PR Require a Contrib Repo Change?
Checklist: