Skip to content

Conversation

@eweitz
Copy link
Member

@eweitz eweitz commented Jul 23, 2020

This improves observability for Ingest Pipeline by logging errors to Sentry, our main error monitoring platform.

Previously, Ingest Pipeline errors were only 1) logged to an ingest-operation-specific errors.txt file, and 2) emailed in summary form to the user.

Now, such errors are also logged to Sentry. This lets us easily correlate and inspect errors in Ingest Pipeline, using stack traces and other rich debugging context, in the same place we use for our main Rails codebase. You can find example Ingest Pipeline errors in Sentry by filtering on logger:ingest_pipeline. Ingest Pipeline errors also appear in the default unfiltered view.

I tested this manually by setting the SENTRY_DSN environment variable in my terminal (as described in the updated README instructions) and ingesting known-bad files via example commands noted atop ingest_pipeline.py. I also manually tested the integration by pointing my local SCP Rails app to a development Docker build in GCR (gcr.io/broad-singlecellportal-staging/scp-ingest-pipeline-development:1.4.0_deb29ad).

See broadinstitute/single_cell_portal_core/pull/675 for a small, coupled PR.

This satisfies SCP-2525.

@eweitz eweitz changed the title Log errors to Sentry Log errors to Sentry (SCP-2525) Jul 23, 2020
@codecov
Copy link

codecov bot commented Jul 23, 2020

Codecov Report

Merging #117 into master will increase coverage by 0.07%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
+ Coverage   50.19%   50.27%   +0.07%     
==========================================
  Files          20       20              
  Lines        2526     2538      +12     
==========================================
+ Hits         1268     1276       +8     
- Misses       1258     1262       +4     
Impacted Files Coverage Δ
ingest/ingest_pipeline.py 48.64% <ø> (ø)
ingest/monitor.py 85.24% <66.66%> (-4.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59f4042...8d7f471. Read the comment docs.

Copy link
Contributor

@jlchang jlchang left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@devonbush devonbush left a comment

Choose a reason for hiding this comment

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

code looks great, just minor comment updates

See logs in Sentry:
https://sentry.io/organizations/broad-institute/issues/?project=1424198
'''
Copy link
Contributor

Choose a reason for hiding this comment

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

I love these links to the docs. I might move them up to where you import sentry, since these are more general

eweitz and others added 2 commits July 24, 2020 13:22
Co-authored-by: Devon <dbush@broadinstitute.org>
@broadinstitute broadinstitute deleted a comment from devonbush Jul 24, 2020
@eweitz eweitz merged commit 92d8971 into master Jul 27, 2020
@bistline bistline deleted the ew-sentry-error-log branch May 21, 2021 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants