Skip to content

Add jaeger option to split oversized batches #1475

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Add jaeger option to split oversized batches #1475

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions specification/sdk-environment-variables.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ See [OpenTelemetry Protocol Exporter Configuration Options](./protocol/exporter.
| OTEL_EXPORTER_JAEGER_ENDPOINT | HTTP endpoint for Jaeger traces | <!-- markdown-link-check-disable --> "http://localhost:14250"<!-- markdown-link-check-enable --> |
| OTEL_EXPORTER_JAEGER_USER | Username to be used for HTTP basic authentication | - |
| OTEL_EXPORTER_JAEGER_PASSWORD | Password to be used for HTTP basic authentication | - |
| OTEL_EXPORTER_JAEGER_AGENT_SPLIT_OVERSIZED_BATCHES  | Split jaeger exporter UDP batches if they exceed the UDP packet limit | false                                                                                               |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should this be an environment variable? Can you first define this in the jaeger exporter document? https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk_exporters/jaeger.md

You should explain this configuration option then for that config option we can add an env variable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split jaeger exporter UDP batches if they exceed the UDP packet limit

This doesn't seem like it should be configurable. If it's set to false, what's the solution?

In Jaeger own SDKs, there was no batch size parameter (except for Python, but for different technical reasons), there is only packet size, and the exporter always flushes appropriately sized packets (since exceeding the size means the agent will reject them).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it configureable because it might have a performance impact to further-fragment the batches. I can life without it being configureable but this should be decided by the maintainers of the python-sdk.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this is a conflict between batching done above the exporter and batching done by the exporter itself. Shared/reusable batching (by # of spans) makes sense only for transports that do not impose strict message size limits. In case of UDP transport with fixed max message size the batching should be really in the transport itself, not above it with different size semantics.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is merged in the python implementation now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janLo Could you please sum up the reasons brought up by the Python maintainers for making it configurable? This could help in convincing @yurishkuro. I glanced over the issue you linked above (open-telemetry/opentelemetry-python#1500) but couldn't find an explanation.

If we can't find an agreement to put this in the spec, it would still be fine to keep this as a Python-specific peculiarity if Python maintainers want to keep it this way nevertheless. In this case, it should be prefixed with PY(THON) as specified here, however: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#language-specific-environment-variables

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing that I need to be convinced of is why there is ever a need to NOT split the batches, since this will simply result in too large UDP packets that will be discarded by the server.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has performance implications to split the packets. One can argue that ending up with too large batches is a configuration error in the first place and we do not want to spend time to send two packets out if we originally planned to send one.

In the end I'm really indifferent to the option being configurable. If you think it's better to be the default I can provide another PR as soon as I find the time. I ran into this b/c I use open telemetry for a e2e test system where I have quite large traces with a lot of metadata. I don't know if other usecases trigger the issue solved with this option at all - and as said, it does add complexity and probably performance implications on the way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So performance implications of splitting batches vs. performance implications of doing all the work of collecting them and then dropping them on the floor because the agent can't process them. Seems like an easy choice :-)


## Zipkin Exporter

Expand Down