Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1148 +/- ##
==========================================
- Coverage 86.85% 85.83% -1.02%
==========================================
Files 62 63 +1
Lines 6092 6184 +92
==========================================
+ Hits 5291 5308 +17
- Misses 587 656 +69
- Partials 214 220 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| clone.breadcrumbs = make([]*Breadcrumb, len(scope.breadcrumbs)) | ||
| copy(clone.breadcrumbs, scope.breadcrumbs) | ||
| clone.breadcrumbs = make([]*Breadcrumb, 0, len(scope.breadcrumbs)) | ||
| for _, b := range scope.breadcrumbs { | ||
| clone.breadcrumbs = append(clone.breadcrumbs, deepCopyBreadcrumb(b)) | ||
| } | ||
| clone.attachments = make([]*Attachment, len(scope.attachments)) | ||
| copy(clone.attachments, scope.attachments) | ||
| for key, value := range scope.tags { | ||
| clone.tags[key] = value | ||
| } | ||
| for key, value := range scope.contexts { | ||
| clone.contexts[key] = cloneContext(value) | ||
| } | ||
| for key, value := range scope.extra { | ||
| clone.extra[key] = value |
There was a problem hiding this comment.
Correct me if I'm wrong, but:
this was here already and was not causing problems, and it's a separate code path from the buffer/transport, so I don't think this should be changed at all.
I assume this was deliberately done this way because scope forking happens frequently and so you wouldn't want to do an expensive deep copy.
| // a proper deep copy: if some context values are pointer types (e.g. maps), | ||
| // they won't be properly copied. | ||
| func cloneContext(c Context) Context { | ||
| res := make(Context, len(c)) |
client.go
Outdated
| if event.User.Data != nil { | ||
| eventToBuffer.User.Data = deepCopyMapStringString(event.User.Data) | ||
| } | ||
|
|
There was a problem hiding this comment.
Nested in Event there are other things we might care about, for example a Span can have Data (and apparently tags and extra), same about Log attributes, etc. Do we care about those?
|
...or we can serialize it up front, and then send them to telemetry buffer after serialization? I would assume doing deep copy would inflict in more RAM usage than just serialize it up front. |
I was weighting both approaches, but i think that pre-serialization has more drawbacks:
That is a valid concern but we only keep the copy around till we flush the event (short lifespan) and other SDKs already copy some event data. |
| } | ||
|
|
||
| for key, value := range scope.extra { | ||
| event.Extra[key] = value | ||
| event.Extra[key] = deepCopyValue(value) | ||
| } | ||
| } | ||
|
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Okay so what's blocking this PR? Just the reviews? |
Validity mostly.
I am also not 100% sure that this is better than preserialization so weighting that as well. |
giortzisg
force-pushed
the
fix/buffers
branch
from
b4d4435 to
8bbb491
Compare
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
giortzisg
force-pushed
the
fix/buffers
branch
from
0ef86ef to
581fd11
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| client.batchMeter = newMetricBatchProcessor(&client) | ||
| client.batchMeter.Start() | ||
| if !options.DisableTelemetryBuffer { | ||
| client.setupTelemetryProcessor() |
There was a problem hiding this comment.
Orphaned HTTPTransport worker goroutine leak on initialization
High Severity
When the telemetry buffer is enabled (the default), setupTransport() creates an HTTPTransport and calls Configure(), which starts a background worker goroutine. Then setupTelemetryProcessor() replaces client.Transport with a new internalAsyncTransportAdapter, orphaning the original HTTPTransport. Since client.Close() only calls Close() on the current client.Transport (the adapter), the original HTTPTransport's worker goroutine is never stopped — its done channel is never closed — causing a permanent goroutine leak on every default client initialization.
Additional Locations (1)
3 participants
Description
Since the addition of Telemetry Buffers moves the serialization to a background worker, user provided attributes that can be mutated should be deep copied, to avoid panics during serialization.