Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1107 +/- ##
==========================================
- Coverage 86.98% 86.78% -0.20%
==========================================
Files 60 62 +2
Lines 5561 6092 +531
==========================================
+ Hits 4837 5287 +450
- Misses 539 590 +51
- Partials 185 215 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
giortzisg
force-pushed
the
feat/telemetry-scheduler
branch
from
6c1fff1 to
2e16e4f
Compare
giortzisg
force-pushed
the
feat/telemetry-scheduler
branch
from
663a9eb to
cda84db
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lcian
left a comment
lcian
left a comment
There was a problem hiding this comment.
Reviewed the diff, Looks Good To Me.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
internal/telemetry/buffer_wrapper.go
Outdated
| // Buffer is the top-level buffer that wraps the scheduler and category buffers. | ||
| type Buffer struct { | ||
| scheduler *Scheduler | ||
| storage map[ratelimit.Category]Storage[protocol.EnvelopeItemConvertible] |
There was a problem hiding this comment.
scheduler already has buffers inside, why the storage here as well?
| }) | ||
| client.Transport = &internalAsyncTransportAdapter{transport: transport} | ||
|
|
||
| storage := map[ratelimit.Category]telemetry.Storage[protocol.EnvelopeItemConvertible]{ |
There was a problem hiding this comment.
storage could be initialized directly inside the scheduler buffers, don't see why we have to expose it?
| categoryToProcess = category | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Unpredictable Map Iteration Causes Unfair Scheduling
Map iteration order is non-deterministic in Go, causing unpredictable buffer selection when multiple buffers share the same priority. When CategoryTransaction and any unknown category (both PriorityMedium) are ready to flush simultaneously, the scheduler randomly picks one, potentially causing unfair scheduling or starvation. The selection should be deterministic, either by sorting categories or using a stable iteration order.
| func (client *Client) Close() { | ||
| if client.telemetryBuffer != nil { | ||
| client.telemetryBuffer.Close(5 * time.Second) | ||
| } |
There was a problem hiding this comment.
Bug: Logger shutdown incomplete, causing resource leaks.
The Close method doesn't shut down batchLogger when it exists. When DisableTelemetryBuffer is true and EnableLogs is enabled, batchLogger is initialized and started, but never properly shut down during Close, potentially causing goroutine leaks and lost log events. The method should call client.batchLogger.Shutdown() when batchLogger is non-nil.
4 participants
Description
Add telemetry scheduler implementation and integrate with the client. The current PR also includes changes on the transport and envelope layer.
The proper solution for batching under the scheduler is to abstract each event type, so that it knows how to convert itself to an envelope item (noted as EnvelopeItemConvertible in the PR), and then the scheduler would just create envelopes using
[]EnvelopeItemConvertible. This partially fixes the issue, since currently everything is anchored around theEventtype. We should also abstract all event types to implementEnvelopeItemConvertiblein the future.Notes
EnvelopeConvertibletoEnvelopeItemConvertibleintroduced in feat: add new envelope transport #1094. For the sake of clarity I added the changes here and not on the other PR. Same applies withToEnvelopeEnvelopeHeader. Left some comments on the implementation.Issues