Make otel scheduler sync #2262
Make otel scheduler sync #2262
Conversation
There was a problem hiding this comment.
The OTel contract states that Start must return quickly, so I suppose sync would be fine. Do we know if the Collector does this async or sync? It'd be best to just do what it does, just to be on the safe side.
Regarding the issue with the batch processor - does it happen because something is wrong on the OTel side, or because #2027 maybe doesn't work for some edge case?
|
It might be good for @thampiotr to opine here, given that this PR reverts changes in #2027. |
It's done sync in the otel collector. The service starts everything sync (https://github.com/open-telemetry/opentelemetry-collector/blob/main/service/service.go#L230) and it's called sync here (https://github.com/open-telemetry/opentelemetry-collector/blob/main/otelcol/collector.go#L228)
Piotr and I had a discussion about it here: https://raintank-corp.slack.com/archives/C02GSU8SHBN/p1733846257348439 |
It doesn't revert all the changes, it's reusing the approach. |
| // A message is already queued for refreshing running components so we | ||
| // don't have to do anything here. | ||
| } | ||
| level.Debug(cs.log).Log("msg", "scheduling components", "count", len(cc)) |
There was a problem hiding this comment.
| level.Debug(cs.log).Log("msg", "scheduling components", "count", len(cc)) | |
| level.Debug(cs.log).Log("msg", "scheduling otelcol components", "count", len(cc)) |
It's quite an ambiguous log line, but if you think there is value in keeping it I'm happy to have it as a debug log.
* Don't start components until Run is called * Update consumers after stopping the component * Minor fixes
The fix in #2027 did not address all the race conditions that could happen with the scheduler. We noticed it when getting rid of our Otel fork brought back the flakiness of a batch processor test.
With this change, the scheduling of otel components is not an async operation anymore. This simplifies the logic and ensures that the components are started before they can start consuming.