API change: Capture methods should not return *EventID (*string)

[rhcarvalho]

In the last months I've seen in user code a few misuses of the return value of CaptureException and related. Seen again today, formalizing the problem here.

This is an example: https://github.com/ruslanBik4/httpgo/blob/@%7B2020.03.02%7D/logs/writer.go#L124-L126

message = string(*(sentry.CaptureException(err)))

If sentry.CaptureException returns nil, and it is documented to return nil "in case Sentry is disabled or event was dropped", user code will panic.

I think this is a case where the zero-value (empty string) would make it easier to use the API (or rather harder to misuse).

Perhaps always returning a non-empty string is also reasonable.

And we may as well drop the EventID type altogether.

---

• [ ] Investigate the behavior of other SDKs: how do they represent EventID; is it ever null?.

[rhcarvalho]

This is another example https://github.com/getsentry/sentry-go/issues/98#issue-530312849

[rhcarvalho]

gitlab.com/gitlab-org/gitaly/internal/dontpanic/retry.go
32-
33-     func() {
34-             defer func() {
35-                     recovered = recover()
36-                     if err, ok := recovered.(error); ok {
37:                             id = sentry.CaptureException(err)
38-                     }
39-             }()
40-             fn()
41-     }()
42-
43-     if id == nil || *id == "" {
44-             return
45-     }
46-
47-     logger.WithField("sentry_id", id).Errorf(
48-             "dontpanic: recovered value sent to Sentry: %+v", recovered,
49-     )

The code above does nothing when id is nil, even though it may also have recovered from a panic.

[rhcarvalho]

github.com/anshap1719/go-authentication/utils/log/log.go
18-                     res, err := e(ctx, req)
19-
20-                     if err != nil {
21-                             reqID := ctx.Value(middleware.RequestIDKey).(string)
22-
23:                             event := sentry.CaptureException(errors.WithMessagef(err, "[%s]: [%s] ERROR: %s", prefix, reqID, err.Error()))
24-                             eventID := string(*event)
25-
26-                             sentry.Flush(time.Second * 5)

The code above will panic when event is nil.

It also shows a misuse of sentry.Flush.

The same pattern repeats in here:

github.com/anshap1719/go-authentication/cmd/auth/http.go
192-    return func(ctx context.Context, w http.ResponseWriter, err error) {
193-            id := ctx.Value(middleware.RequestIDKey).(string)
194-            w.Write([]byte("[" + id + "] encoding: " + err.Error()))
195-            reqID := ctx.Value(middleware.RequestIDKey).(string)
196-
197:            event := sentry.CaptureException(errors.WithMessagef(err, "[%s] ERROR: %s", reqID, err.Error()))
198-            eventID := string(*event)
199-
200-            sentry.Flush(time.Second * 5)
201-
202-            w.Header().Add("Sentry-Event-ID", eventID)

[rhcarvalho]

github.com/cockroachdb/errors/report/report.go
202:    res := sentry.CaptureEvent(event)
203-    if res != nil {
204-            eventID = string(*res)
205-    }
206-    return

User code typically have to convert to string anyway... and only sometimes people get it right.

[rhcarvalho]

github.com/Vernacular-ai/vcore/surveillance/sentry.go
81-                             // Capturing the error on Sentry
82:                             eventId := sentry.CaptureException(err)
83-                             log.Errorf(err, "Error captured in sentry with the event ID `%s`", *eventId)

Line 83 above will panic when eventId is nil. Innocent-looking code that crashes.

[ste93cry]

getsentry/sentry-javascript#2049 may be relevant in the decision of what to do in this SDK as well as in the others in regards to what the capture* methods should return

[rhcarvalho]

@ste93cry for the current form of the Unified API, my claim is that returning string is less error prone and more ergonomic than *string (in fact *EventID) in Go.

I'm not considering anything like a "promise" as that's not idiomatic Go. Did you have something more specific in mind?

[ste93cry]

I just started learning Go so I cannot really speak about it, I just referenced an issue in another SDK that it talking about a similar problem of what to return from the capture* methods. I think that what it seems to be as similar as possible to the concept of returning a promise or a future is returning a Go channel: I personally really really like the SierraSoftworks/sentry-go package as I think that it provides some neat client-related APIs that the official SDKs should learn from

[rhcarvalho]

@ste93cry thanks for the note.

Allocating a new channel for every event is wasteful on resources, specially considering that the common case is not to "wait" on the delivery of each event (this is true regardless of the language).

If your use case can afford waiting on network roundtrips for error reporting, then we cover this out-of-the-box with the HTTPSyncTransport transport implementation, and the same reporting API.

I'm personally always happy to get feedback and appreciate ideas how we can improve the SDK(s), and I feel comfortable to say it is the same for other teammates at Sentry. I have opened issues with ideas myself and implemented some as time permits. Please feel free to open new issues if you have specific suggestions or comment on existing ones! User feedback helps us prioritize our work. Many thanks!

[ste93cry]

Allocating a new channel for every event is wasteful on resources, specially considering that the common case is not to "wait" on the delivery of each event (this is true regardless of the language)

The biggest problem with the current API is that the API is asyncronous under the hood and syncronous on the surface: this leads to a lot of problems like:

• people think that the event has been sent after the capture* method returns to the caller while instead the event may still be in the queue
• there is no way to know if the event has been sent or not, and the event ID is useless because it's generated client-side and so it's not something that people should rely on for answering this question

[joshma]

Not to be just a +1 on this, but I ran into this today too - would be nice to have this just be a normal string, with "" instead of nil. I'll point out this matches hub.LastEventID() anyways.

For my case, it's annoying because I want to put this in an error log message after:

eventID := sentry.CaptureException(err)
log.Errorf("error while doing something (eventid=%s): %s", eventID, err)

But this doesn't work because eventID is a string-pointer. I'd have to check for nil and de-reference everytime I want to use it. Probably going to just get rid of including eventID for now.

[ArcticSnowman]

EventID is always returned regardless of the request outcome to Sentry.IO

[rhcarvalho]

EventID is always returned regardless of the request outcome to Sentry.IO

That's because the IDs are generated on the client, and there are no strong guarantees about event ingestion (e.g. even if you get a 200 from the ingestion endpoint, the event may still not be persisted -- many reasons for that, from user-configured filters to ephemeral pipeline processing problems).

[github-actions[bot]]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀