bug: add tag info to sentry error messages

[jrconlin]

Description

Not a super fan of this. It might be better as middleware, but that's
not always defined when we make sentry error calls. Passes a Tags structure around that contains the tags that will be added to sentry and metrics calls.

Testing

There is an included unit test to check default tag generation.

In addition, there is now a new test endpoint called /__error__ that will generate a sentry error containing the ua tags. (see https://sentry.prod.mozaws.net/operations/syncstorage-dev/issues/6888305/ for sample output)

When running tests, note that the following tags should be added to sentry and metric calls:

• ua,os,ver
• ua.os.family
• ua.browser.ver
• ua.name
• ua.browser.family
• uri.path
• uri.method

Issue(s)

Issue: #329

[jrconlin]

based on #373 need to make sure tag/"extra" fields are actually sent to sentry.

[tublitzed]

@jrconlin - to confirm for the testing bit of this; have we verified manually in sentry that these tags appear as expected?

[jrconlin]

@tublitzed They've not and thus the comment.

[tublitzed]

I just tested this locally against sentry with this test event - I'm not seeing any of these new tags.

Here's the tags I am seeing in Sentry:

If some tags wind up being harder to pass along than others, it might be a good idea to just start by adding the ones we really need right now to start with, and coming back in later for a more complete pass.

[jrconlin]

Right, there needs to be an explicit sentry:: capture call similar to https://github.com/mozilla-services/syncstorage-rs/pull/373/files#diff-b7289185b77ef7668febb777d5d33c5fR249. My initial misunderstanding was that the sentry tag data was being pulled from the logging messages directly, that's why I am currently trying to fix the issue with my local instance communicating with sentry directly.

[tublitzed]

@jrconlin I'm not seeing any change between here and master related to errors appearing in Sentry.

Here's what I'm doing:

1. Adding a panic to line 56 of main.rs.
2. Starting server like this (SENTRY_DSN={DSN__HERE} make run_local)

The errors generated from this branch appear identical to what's coming from master. I'm confused around what you've changed here specifically related Sentry tags; I'm assuming that I'm just missing something obvious here....would you mind clarifying specifically the new fields that you've added to Sentry, and how we can test this to make sure they're appearing as expected?

For reference, here are sentry errors from this branch vs sentry errors from master - AFAIK there is no new information there?

Ie, all the tags look identical.

[jrconlin]

@tublitzed Unfortunately, panics (and other macro based errors such as error!, warn!, etc.) require special effort in order to transmit tag info. This is because those tend to strip off tag info or happen outside of the contexts where the Tags structure are defined. There are some examples in the code base, and there might be additional ways that we could automate some of that, but for now, this patch adds the tagging info to the ApiError messages we generate normally. (See https://github.com/mozilla-services/syncstorage-rs/pull/372/files#diff-9de2327cbb67a90931e98ff70715da9dR570 for an example of explicit tag set, however be aware that sentry currently does not transmit those additional tags values.)

While not perfect, it gets us further than where we were without the tag info.

[tublitzed]

@jrconlin - makes sense to me. We can come back later to try and address panics etc if/when needed. Thanks for the example of the specific error; can you let those of us reviewing this PR know how to trigger that error so we can verify that it's passing along the correct info into Sentry?

[jrconlin]

Yes, I updated the instructions above to indicate the new /__error__ endpoint (sorry, the original got changed by markdown to /error)

[tublitzed]

@jr can you highlight what's different in your example event than what we get from master? I'm not seeing any new tags in Sentry.

I tried it locally too, here's what I get:

this is from this branch, using the __error__ endpoint:

this is from this branch, in master using a panic:

...AFAIK there isn't a difference in the tags sent. Maybe I'm looking in the wrong place in the Sentry UI?

[jrconlin]

??? now I'm not seeing the added tags either.

[tublitzed]

Verified locally that I'm seeing the tags with the test endpoint.

[jrconlin]

@pjenvey points out that it's possible to downcast to ApiError to avoid the problem:

    diff --git a/src/web/middleware.rs b/src/web/middleware.rs
    index 645ab41769..3eda353d59 100644
    --- a/src/web/middleware.rs
    +++ b/src/web/middleware.rs
    @@ -368,15 +368,12 @@ where
                                 tags.tags.insert(k, v);
                             }
                         };
    -                    let mut event = Event::new();
    -                    // The following cannot safely be shared between threads.
    -                    // note this does mean that the backtrace could be lost.
    -                    // let mut event = integrations::failure::event_from_fail(e);
    -                    if let Some(r_err) = e.as_error::<ApiError>() {
    -                        event.message = Some(format!("{:?}", r_err.kind()));
    +                    let apie: Option<&ApiError> = e.as_error();
    +                    if let Some(apie) = apie {
    +                        let mut event = sentry::integrations::failure::event_from_fail(apie);
    +                        event.tags = tags.as_btree();
    +                        sentry::capture_event(event);
                         }
    -                    event.tags = tags.as_btree();
    -                    sentry::capture_event(event);
                     }
                 }
                 resp

Also might be worth digging into sentry_actix to see how it handles passing the cause.backtrace