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

Check if we need to make changes to support ForceFlush in batch processor #100

Closed
Assignees

Comments

@nilebox
Copy link
Contributor

nilebox commented Sep 21, 2020

See recent change open-telemetry/opentelemetry-go#1166 (issue: open-telemetry/opentelemetry-go#1072)

@jsuereth jsuereth self-assigned this Mar 2, 2021
@jsuereth
Copy link
Contributor

jsuereth commented Mar 2, 2021

TL;DR: Our Exporter makes the assumption that it will be offloading all spans into batches and exporting as a group.

OpenTelemetry Exporters are not allowed to do this by specification (for some reason). This means that users of ForceFlush could wind up in a state where some spans are not exported by the GCT exporter.

The only resolution here would be to "force flush" on every export call. Given this code is also used in the collector that gives me pause. Here's a proposed solution:

  1. Update the configuration for the trace exporter with an option "batch_spans" which defaults to false (going forward)
  2. If "batch_spans" is false, then we call Flush on bundler after every export.
  3. When we configure this exporter in the collector, we set batch_spans to true.

@dashpole
Copy link
Contributor

dashpole commented Mar 2, 2021

I think the collector should do batching using the exporter helper. IIRC it doesn't belong in our exporter here.

@jsuereth
Copy link
Contributor

jsuereth commented Mar 2, 2021

Sure, I'll look at the exporter-helper batching in the collector and if I can migrate to that quickly, I will. However, I think the TL;DR: is that we'll have a long-term plan to remove the bundler from this exporter, but we may just call Flush for the next release to avoid churning the design.

@punya
Copy link
Contributor

punya commented Mar 2, 2021

@jsuereth given that the upstream OT-Go release is still ~2 weeks away, could we consider spending the time to actually remove the bundler now instead of deferring that work?

@ymotongpoo
Copy link
Member

At the timing of direct exporter implementation, the spec on batching wasn't fixed and I followed the design in OC exporter. I have no objection to remove bundler.

This was referenced Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants