feat: Allow some non-Golang interfaces to be passed through in Events.

[jwise]

Some users may wish to use sentry-go as a mechanism to proxy non-Golang events -- for instance, if reporting errors from a mobile or web application. We add Sentry event payload interfaces that make it easier to support other platforms, including stack frame interface members for "C-like" applications, and including the Debug Meta interface that allows the Sentry backend to symbolicate application backtraces.

This change does not include test changes, because the JSON serialization interface does not appear to presently have other test coverage.

This change was authored under contract for FullStory.

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: +0.15 🎉

Comparison is base (85b380d) 78.83% compared to head (8b47c36) 78.99%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #606      +/-   ##
==========================================
+ Coverage   78.83%   78.99%   +0.15%     
==========================================
  Files          38       38              
  Lines        3851     3851              
==========================================
+ Hits         3036     3042       +6     
+ Misses        712      708       -4     
+ Partials      103      101       -2     

Impacted Files	Coverage Δ
interfaces.go	93.33% <ø> (ø)
stacktrace.go	83.65% <ø> (+1.44%)	⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[cleptric]

I'm not quite sure why you would proxy events through the Go SDK.
All of our SDKs offer an async transport, which makes sending events to Sentry pretty straightforward.

[jwise]

@cleptric asked:

I'm not quite sure why you would proxy events through the Go SDK. All of our SDKs offer an async transport, which makes sending events to Sentry pretty straightforward.

In our case, our mobile and web apps (iOS, Android, web, ...) do their own logging and then communicate events through an endpoint on our own servers, which then relays events up to Sentry (and, previously, to our previous error reporting provider). We'd like to keep this behavior (and we'd like to maintain compatibility with our clients in the field, rather than having to ship new client binary versions to support Sentry). Most of our back end -- including our exception relay endpoint, /rec/except -- is written in Golang, so reusing the existing sentry-go infrastructure (HTTP client, DSN parsing, structured data, etc) seems more sensible than writing our own Sentry client.

[jwise]

Force pushed to fix linter complaints. (I am not a native Gopher, so please feel free to correct me on any other style issues!)

[cleptric]

Sounds reasonable. Let me run this by some folks internally to check if we run into any issues during ingestion.

[jwise]

Thanks! Here's a snippet of the other workaround we have to make this behave as we'd like:

func mangleSentryMobileEvent(ev *sentry.Event, evhint *sentry.EventHint) *sentry.Event {
        if ev.Tags["device.osName"] == "Android" {
                ev.Platform = "java"
        } else {
                ev.Platform = "cocoa"
        }

        // XXX FUTURE: fill in ev.Release

        ev.Sdk.Name = "fs-recbugs"
        ev.Sdk.Version = "unknown"
        ev.Sdk.Integrations = nil
        ev.Sdk.Packages = nil

        return ev
}

...

        client, err := sentry.NewClient(sentry.ClientOptions{
                ...,
                BeforeSend: mangleSentryMobileEvent,
                Integrations: func([]sentry.Integration) []sentry.Integration { return nil },
                Release: "unset",
        })

[cleptric]

Using BeforeSend is perfectly fine for such "exotic" setups, no problem at all 😄 👍

[jwise]

I just tested this end-to-end and have successfully recorded an event with it.

[jwise]

Hiya @cleptric , any news on whether you'd be able to merge this? We'd really like to get this in upstream so that we can put a build dependency on it. Thanks!

[tonyo]

Hey @jwise , the changes look good from my side, but I'd still like to ask you to add a basic test for the fields you've added. The idea is for those test(s) to fail if some of those new public fields are changed or removed from Event struct in the future (accidentally or not), which would also help us as maintainers to promptly detect that.

Something similar to https://github.com/getsentry/sentry-go/blob/master/interfaces_test.go#L137-L161 would do. Thank you!

After that's done, we can merge it, and it'll be included in the next release, which will go out probably somewhere in the next couple weeks.

[jwise]

Hi @tonyo , thanks for the suggestion for where those could go! I've added tests for a handful of those fields in 810c51d, but not all of them, since I didn't see precedent elsewhere in interfaces_test.go for exhaustively testing an interface, and doing so would make the JSON being tested against rather ungainly. Do you think I should test these exhaustively instead, or is this in line with what you had been hoping for?

(Please forgive me if the JSON string concatenation is unidiomatic. I do not natively speak Go!)

[tonyo]

To sum up, let's move the new additions to two separate tests, where we test just those changes. Otherwise, looks good!

[tonyo]

Please move your additions to a new test (a new function that starts with "Test", e.g. TestMarshalEventWithDebugMeta), and preferably include all newly added fields there (e.g. with two images, "macho" and "proguard", to cover all the fields in DebugMetaImage).
The preamble (event := NewEvent()) will stay the same in the new test, but everything else will be purely about DebugMeta testing.

[jwise]

@tonyo Force-pushed a new version that splits those tests out, and tests every element of the struct. What do you think of these instead?

[tonyo]

Looking good, thanks!

[tonyo]

The GOPATH test failure seems to be unrelated (it's now failing on master too, probably something changed on the GitHub runner side), so merging.

[tonyo]

@jwise this is now released in v0.20.0 👍

[jwise]

Wow, that was amazingly fast! That makes it much easier for me to set a dep on it internally. Thanks so much!!