Skip to content

Resolves #1151 - send errors, error-logs and panics to sentry #1486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Oct 6, 2021

Conversation

syphar
Copy link
Member

@syphar syphar commented Sep 12, 2021

This PR starts sending errors to sentry.

What is sent to sentry now (including backtraces):

  • log::error!
  • anyhow::Error, when ending up in the main handler
  • panic! everywhere

I'm not 100% sure if there need to be more places changed?

open topics / question

  • should all the error! calls in the codebase stay errors being sent to sentry? I could imagine some of them should be changed to warnings? Or should we exclude error-logs from sentry?
  • I would love to switch the handling code in the ctry! to use capture_anyhow when it gets a fitting error type, but I wasn't sure about the best approach. (done, thanks to @jyn514)

@syphar syphar self-assigned this Sep 12, 2021
@syphar syphar added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Sep 12, 2021
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really cool :) do you know how I can test it?

sentry_dsn,
sentry::ClientOptions {
release: Some(docs_rs::BUILD_VERSION.into()),
attach_stacktrace: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, why set this? The docs say:

When enabled, stack traces are automatically attached to all messages logged. Stack traces are always attached to exceptions; however, when this option is set, stack traces are also sent with messages. This option, for instance, means that stack traces appear next to all log messages.

and I don't think it's super useful to show a backtrace for the top-level error! macro, since it will always be the same.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to have automatic stack-traces for log::error! calls.

I might be wrong, but in testing the stacktrace looked correct, from the place where the log::error call was made:

grafik

( I had an additional log::error call in main)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be wrong, but in testing the stacktrace looked correct, from the place where the log::error call was made:

Right - but my point is that usually we want the stacktrace of the error itself, not the error! call.

I guess that's an argument for getting rid of error! altogether and changing all those to capture_anyhow; I would be ok leaving this in until we do that.

@syphar
Copy link
Member Author

syphar commented Sep 12, 2021

This is really cool :) do you know how I can test it?

@jyn514 short answer to this question:
I used my personal sentry account to create a SENTRY_DSN, then manually when through the errors to see what ends up in sentry.

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Sep 13, 2021
@jyn514 jyn514 added the A-admin Area: Administration of the production docs.rs server label Sep 13, 2021
@syphar
Copy link
Member Author

syphar commented Sep 20, 2021

@jyn514 IMHO you can have another look :)

I replaced log::error! with report_error and sometimes log::error! with log::warn!.

Right now I'm not sure

  • if all the replacements are ok to replace
  • if the remaining error! calls can stay like this
  • if there are duplications where the same error would lead to multiple logs/sentry errors in some cases. we could also just fix it when this happens.

Also, from a rust perspective, if there is a better way than the report_error(&anyhow!(err).context(format!( construct.
I only can imagine:

  • a single macro for anyhow with context/format?
  • extending std-err so we can call .context on it, returning an anyhow-err? ( failure had this, anyhow doesn't)

@syphar syphar added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Sep 20, 2021
@syphar syphar requested a review from jyn514 September 20, 2021 19:56
@jyn514
Copy link
Member

jyn514 commented Sep 20, 2021

if there are duplications where the same error would lead to multiple logs/sentry errors in some cases. we could also just fix it when this happens.

Fixing them as they occur seems fine - I'd rather have too many alerts than too few.

if there is a better way than the report_error(&anyhow!(err).context(format!( construct.

Another macro seems fine. I don't know how to do it through traits without messing with it on desktop, and I don't think it needs to block the PR.

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Sep 21, 2021
@syphar syphar requested a review from jyn514 September 22, 2021 21:31
@syphar syphar added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Sep 22, 2021
@syphar
Copy link
Member Author

syphar commented Sep 22, 2021

@jyn514 this is probably ready for another review :)

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :) @Nemo157 feel free to review too if you like, I won't merge this until the next time I do a deploy (so probably no sooner than Saturday).

@jyn514 jyn514 added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Sep 23, 2021
@jyn514 jyn514 merged commit 8e46833 into rust-lang:master Oct 6, 2021
@syphar syphar deleted the sentry branch October 6, 2021 07:35
@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-admin Area: Administration of the production docs.rs server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants