FE-147 add sentry for production readiness interim dagster #326

bahill · 2024-01-29T19:56:24Z

Why

Relevant ticket FE-147

This PR

Checklist

Documentation has been updated as needed.

…ions with better conditional logic (see FE-128)

…tional logic

…Biosphere/hca-ingest into FE-128-dedup-github-actions

…sue. Testing is not needed in dev as we run this on all PR pushes & syncs. Testing can/should be run locally in dev (takes 90 minutes)

…Biosphere/hca-ingest into FE-128-dedup-github-actions

…to FE-147-add-sentry-for-production-readiness-interim-dagster

…nterim-dagster

sonarqubecloud · 2024-01-29T19:57:38Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

codecov · 2024-01-29T20:06:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ed8a144) 95.56% compared to head (bc271be) 95.56%.
Report is 10 commits behind head on main.

❗ Current head bc271be differs from pull request most recent head ce167b5. Consider uploading reports for the commit ce167b5 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #326   +/-   ##
=======================================
  Coverage   95.56%   95.56%           
=======================================
  Files           4        4           
  Lines         158      158           
  Branches        5        5           
=======================================
  Hits          151      151           
  Misses          7        7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aherbst-broad

Had a question about the SDK init. Can you fix up the description a bit too? It seems to be mis-formatted.

aherbst-broad · 2024-01-30T15:54:43Z

orchestration/Dockerfile

@@ -9,7 +9,8 @@ ENV PYTHONFAULTHANDLER=1 \
  PIP_NO_CACHE_DIR=off \
  PIP_DISABLE_PIP_VERSION_CHECK=on \
  PIP_DEFAULT_TIMEOUT=100 \
-  POETRY_VERSION=1.1.8
+  POETRY_VERSION=1.1.8 \
+  SENTRY_DSN=https://[email protected]/4506559533088768


Does this mean the pipeline is using the same DSN for dev and prod? Will that intermix errors between the two environments or is there a way to distinguish between the two the sentry setup?

Yes - Sam has a setup for this whereby each env gets a DSN which I like, but seemed like overkill for a throwaway. Plus, the only time dev runs is when I am testing the pipeline and hopefully don't have to do that again?
And - I'm still unsure of the utility here as it's all just duplicating the dagit errors which are also caught in the cloud console logs explorer.
What do you think?

Yeah, I'm not thrilled with the idea of intermixing the environments but given the time constraints and desire to replatform probably not worth revisiting at this point.

FWIW - if it becomes an issue in this interim, I will address it and definitely will not intermix in the re-platform

aherbst-broad · 2024-01-30T15:57:47Z

orchestration/hca_orchestration/pipelines/cut_snapshot.py

+    "",
+)
+if SENTRY_DSN:
+    sentry_sdk.init(dsn=SENTRY_DSN, traces_sample_rate=1.0)


The docs state that the sdk will attempt to pull from the environment automatically and if it can't find the DSN it will just not send events. Given that, what do you think about simplifying this logic to a sentry_sdk.init(traces_sample_rate=1.0) and letting the SDK sort out the env var resolution?

That makes sense to me - I just copied this in from what Zebrafish & SCP did. - I don't have strong feelings either way.

Yeah Bond does it too
https://github.com/DataBiosphere/bond/blob/e53378b1563ef2275ff66b9e924c0a3f776ac711/main.py#L21

At this point... I'm inclined to leave it alone unless I need to touch this again. I'd have to rebuild, deploy to dev, test, then PR to main, which triggers the E2E tests, and then go through review and deploy to prod, then smoke test. That functionally takes me to the end of the week.
What do you think about the compromise of making a ticket for this (it's what I did with cleaning up the git actions)? I'll pick this back up if I have to update the codebase again?

Sure, ticket makes sense.

aherbst-broad

🚢

aherbst-broad · 2024-01-30T20:48:28Z

orchestration/hca_orchestration/pipelines/cut_snapshot.py

+    "",
+)
+if SENTRY_DSN:
+    sentry_sdk.init(dsn=SENTRY_DSN, traces_sample_rate=1.0)


Sure, ticket makes sense.

aherbst-broad · 2024-01-30T20:49:49Z

orchestration/Dockerfile

@@ -9,7 +9,8 @@ ENV PYTHONFAULTHANDLER=1 \
  PIP_NO_CACHE_DIR=off \
  PIP_DISABLE_PIP_VERSION_CHECK=on \
  PIP_DEFAULT_TIMEOUT=100 \
-  POETRY_VERSION=1.1.8
+  POETRY_VERSION=1.1.8 \
+  SENTRY_DSN=https://[email protected]/4506559533088768


Yeah, I'm not thrilled with the idea of intermixing the environments but given the time constraints and desire to replatform probably not worth revisiting at this point.

orchestration/Dockerfile

kilonzi · 2024-01-30T14:46:58Z

orchestration/hca_orchestration/pipelines/validate_ingress.py

+    "",
+)
+if SENTRY_DSN:
+    sentry_sdk.init(dsn=SENTRY_DSN, traces_sample_rate=1.0)


Do you know or can you test that if you had a file say sentry_entrypoint.py and in that if you defined the
SENTRY_DSN = os.getenv( "SENTRY_DSN", "", ) if SENTRY_DSN: sentry_sdk.init(dsn=SENTRY_DSN, traces_sample_rate=1.0)
and that if in each of the files where you want sentry to monitor then you wrote
**from sentry_entrypoint import *** if that would work too?

@bahill I was trying to find a way of not repeating if SENTRY_DSN: everywhere

100% with you on that. I weighed it out - doing something like SCP does, but it only saved me like 4 lines of code, and I'm not changing this again, so I went with the brute force duct tape approach.

Also - re: Drew's point from above - it's basically the same number of lines and actually fewer files if I were to leave out the if SENTRY_DSN bit and let Sentry just figure out its DSN from the env

zbedo · 2024-01-30T21:35:25Z

@bahill can we setup SENTRY_DSN as an env var in another PR?

bahill and others added 30 commits January 16, 2024 12:55

Updated manifest.py with testing info, and updated dev & main git act…

f2b6065

…ions with better conditional logic (see FE-128)

removing test data & updating build & publish yamls with better condi…

e30dd50

…tional logic

ignore test data

e2c5218

Merge branch 'FE-128-dedup-github-actions' of https://github.com/Data…

56080db

…Biosphere/hca-ingest into FE-128-dedup-github-actions

removing test data

acbc137

fixing git action syntax

2d0fe27

renaming with underscores

d241965

updated syntax for ignoring branches and improved name

76e0324

Updating dev action to not run tests, and main name to fix display is…

f5d1f96

…sue. Testing is not needed in dev as we run this on all PR pushes & syncs. Testing can/should be run locally in dev (takes 90 minutes)

fixing syntax

59f94af

Merge 59f94af into c5626c4

91e5910

Update requirements.txt

4bcd722

give name to validation job so it can be required by main as a check

c3190dd

Merge branch 'FE-128-dedup-github-actions' of https://github.com/Data…

390852c

…Biosphere/hca-ingest into FE-128-dedup-github-actions

added Sentry to dependencies and Docker env

a9f06ba

adding fetch depth 0 for sonar cloud

a191fe0

First attempt to integrate sentry - test 1

c422483

Merge branch 'main' of https://github.com/DataBiosphere/hca-ingest in…

29d992d

…to FE-147-add-sentry-for-production-readiness-interim-dagster

fixed left over from merge conflict resolution

c4910d9

Adding fetch depth

b2a918a

importing os - maybe this is why the repo didn't load?

b9d66a0

linting

b328575

Merge branch 'main' into FE-147-add-sentry-for-production-readiness-i…

baf8221

…nterim-dagster

Flake8 lint - needed two spaces before my func def

182f2ec

Adding Sentry to our active pipelines

80ec976

linting

c93a2fa

linting

3de4055

Adding config for pylint - ignoring check for unpacking parameters

bc271be

Merge bc271be into 4ed2d88

d33785d

Update requirements.txt

ce167b5

bahill requested review from zbedo, aherbst-broad, kilonzi and ncalvanese1 January 29, 2024 21:11

bahill marked this pull request as ready for review January 29, 2024 21:12

aherbst-broad reviewed Jan 30, 2024

View reviewed changes

bahill requested a review from aherbst-broad January 30, 2024 20:09

aherbst-broad approved these changes Jan 30, 2024

View reviewed changes

kilonzi reviewed Jan 30, 2024

View reviewed changes

kilonzi approved these changes Jan 30, 2024

View reviewed changes

bahill merged commit d73a47e into main Jan 30, 2024
1 check passed

bahill deleted the FE-147-add-sentry-for-production-readiness-interim-dagster branch January 30, 2024 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FE-147 add sentry for production readiness interim dagster #326

FE-147 add sentry for production readiness interim dagster #326

bahill commented Jan 29, 2024 •

edited

Loading

sonarqubecloud bot commented Jan 29, 2024

codecov bot commented Jan 29, 2024

aherbst-broad left a comment

aherbst-broad Jan 30, 2024

bahill Jan 30, 2024

aherbst-broad Jan 30, 2024

bahill Jan 30, 2024

aherbst-broad Jan 30, 2024

bahill Jan 30, 2024

bahill Jan 30, 2024

bahill Jan 30, 2024

aherbst-broad Jan 30, 2024

aherbst-broad left a comment

aherbst-broad Jan 30, 2024

aherbst-broad Jan 30, 2024

kilonzi Jan 30, 2024

kilonzi Jan 30, 2024

bahill Jan 30, 2024

bahill Jan 30, 2024 •

edited

Loading

zbedo commented Jan 30, 2024

FE-147 add sentry for production readiness interim dagster #326

FE-147 add sentry for production readiness interim dagster #326

Conversation

bahill commented Jan 29, 2024 • edited Loading

Why

This PR

Checklist

sonarqubecloud bot commented Jan 29, 2024

Quality Gate passed

codecov bot commented Jan 29, 2024

Codecov Report

aherbst-broad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aherbst-broad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bahill Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

zbedo commented Jan 30, 2024

bahill commented Jan 29, 2024 •

edited

Loading

bahill Jan 30, 2024 •

edited

Loading