Skip to content
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.

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

Remove the bundler from the Jaeger exporter #1799

Closed
3 tasks done
MrAlias opened this issue Apr 12, 2021 · 4 comments · Fixed by #1830
Closed
3 tasks done

Remove the bundler from the Jaeger exporter #1799

MrAlias opened this issue Apr 12, 2021 · 4 comments · Fixed by #1830
Assignees
Labels
pkg:exporter:jaeger Related to the Jaeger exporter package
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Apr 12, 2021

The Jaeger exporter currently uses "google.golang.org/api/support/bundler".Bundler to batch OpenTelemetry span data:

err := e.bundler.AddWait(ctx, span, 1)

The bundler was originally added to the exporter prior to the BatchSpanProcessor existing, and it was used to batch the transformed Jaeger spans. Now that the project contains a BatchSpanProcessor and will send batches of span data to exporters, instead of one-by-one, this batching will cause an impedance mismatch as well as additional cognitive load determining which batching scheme to use.

The bundler, by design, is used to "amortizes an action with fixed costs over multiple item". While this amortization is likely not being utilized given we are sending the exporter a batch and then sequentially adding it to the bundler, we still should verify that removing it does not drastically change the resource overhead or performance.

Needed changes

  • Remove the bundler
  • Remove the associated bundler configuration
  • Validate that this does not add excessive resource consumption overhead
@MrAlias
Copy link
Contributor Author

MrAlias commented Apr 12, 2021

With the bundler in place it was masking the "data too large for a UDP frame error":

return fmt.Errorf("data does not fit within one UDP packet; size %d, max %d, spans %d",

If the bundler was configured to have a buffer larger than this frame size that error is returned for every attempt to send. Removing the bundler has exposed this error because now any testing is not being bundled, though production use of the BatchSpanProcessor would be, and it is exposing this error.

It seems like the UDP sends may need to be split or batched intelligently.

@Aneurysm9
Copy link
Member

It seems like the UDP sends may need to be split or batched intelligently.

Perhaps the use of the bundler should be shifted there and changed to properly track the size of objects added to the bundle. I always thought it a bit odd that the bundler was effectively just being used to count the number of spans and not as it was intended.

@MrAlias
Copy link
Contributor Author

MrAlias commented Apr 13, 2021

Perhaps the use of the bundler should be shifted there and changed to properly track the size of objects added to the bundle. I always thought it a bit odd that the bundler was effectively just being used to count the number of spans and not as it was intended.

Yeah, that could be a useful use of the bundler. It looks like it might be a bit more complex because it is sending the thrift buffer which may not map one-to-one with the spans we enqueue to the bytes buffered. This will make it a bit opaque from our perspective of what size would fit into a frame.

I'm realizing that since this has always been in the code and given we recommend to use the BatchSpanProcessor in production, which should batch effectively and not hit this error either for reasonable batch sizes, we can track the issue and move forward with this.

@MrAlias
Copy link
Contributor Author

MrAlias commented Apr 20, 2021

With the bundler in place it was masking the "data too large for a UDP frame error":

return fmt.Errorf("data does not fit within one UDP packet; size %d, max %d, spans %d",

If the bundler was configured to have a buffer larger than this frame size that error is returned for every attempt to send. Removing the bundler has exposed this error because now any testing is not being bundled, though production use of the BatchSpanProcessor would be, and it is exposing this error.

It seems like the UDP sends may need to be split or batched intelligently.

Tracking with #1828

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:exporter:jaeger Related to the Jaeger exporter package
Projects
None yet
2 participants