Feat/opt automator add region #54631

kneeyo1 · 2023-08-11T15:18:53Z

Added region to the options automator.
for STs "SENTRY_REGION" doesn't work so I'm calling it from "CUSTOMER_ID".
If theres a better way to get the ST region I'm open to it.

kneeyo1 · 2023-08-11T15:48:10Z

src/sentry/conf/server.py

@@ -3660,5 +3660,11 @@ def build_cdc_postgres_init_db_volume(settings: Any) -> dict[str, dict[str, str]
 # are changed by sentry configoptions.
 OPTIONS_AUTOMATOR_SLACK_WEBHOOK_URL: Optional[str] = None

+# This setting is specific to the options automator. It gets the current region or
+# customer in the case of single tenant.
+SENTRY_REGION_OR_CUSTOMER: Optional[str] = (


could also just make it

CUSTOMER_ID = os.environ.get("CUSTOMER_ID" , None)

and call SENTRY_REGION as its already defined in server.py?

Two issues here.

I am a little skeptic that we do not already have a unified way to identiy ST customers or regions. I'd expect Hybrid Cloud would have already dealt with this problem. Mind checking in with them if there is arleady a library somewhere that returns the tenant/region ?

If such api to get the region/tenant does not exist, please do not register this unified value as a setting.
By registering something as a setting you are stating that the value has a default but can be customized and overridden. Which is not what you want. What you want is that sentry_region_or_customer is always either sentry_region or customer_id or None. So you never want this to be overridden to something else.
Thus please move this as a variable or constant inside the presenter that needs it.

Other argument against a dedicated setting. Thiw would add logic into the settings file. This would have unintended consequences that you do not want. It implies the setting file must always be a python file or be able to store arbitrary logic, which makes it less like a setting file and make it hard to move to something like a json/yaml file.

codecov · 2023-08-11T16:06:45Z

Codecov Report

Merging #54631 (d9f6c41) into master (3d47454) will increase coverage by 0.01%.
Report is 45 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #54631      +/-   ##
==========================================
+ Coverage   79.74%   79.75%   +0.01%     
==========================================
  Files        4995     5001       +6     
  Lines      212236   212309      +73     
  Branches    36180    36185       +5     
==========================================
+ Hits       169250   169337      +87     
+ Misses      37777    37760      -17     
- Partials     5209     5212       +3

Files Changed	Coverage Δ
src/sentry/conf/server.py	`91.65% <100.00%> (ø)`

... and 37 files with indirect coverage changes

fpacifici · 2023-08-11T16:54:37Z

The test failure is relevant. Please address that

fpacifici · 2023-08-11T16:55:25Z

src/sentry/runner/commands/configoptions.py

@@ -31,7 +31,7 @@ def _attempt_update(
        if hide_drift:
            presenter_delegator.drift(key, "")
        else:
-            presenter_delegator.drift(key, safe_dump(db_value_to_print))
+            presenter_delegator.drift(key, db_value_to_print)


Is this related ?

fpacifici · 2023-08-11T16:57:01Z

src/sentry/runner/commands/presenters/slackpresenter.py

        json_data = {
+            "region": region,


What happens if you deploy this first and this key is passed to the webhook in eng-pipes before the change to eng-pipes goes out? Would the key be safely ignored or would the webhook break.

In other words: which is the right order to deploy changes here and eng-pipes to ensure backwards compatibility and that nothing breaks at any point in time?

Ah I'll add the deployment process to the slack as well but:
I made it so that if region is not given eng pipes prints messages as is, that way deployment order is not important.

But Ideally, we would deploy here first, eng-pipes would ignore the extra key, I already tested this.
Then we would deploy eng-pipes.
Then we would add the webhook url to getsentry for ST.
Finally we would turn on the option for the slack_webhook in ST.

fpacifici · 2023-08-11T17:06:05Z

src/sentry/conf/server.py

@@ -3660,5 +3660,11 @@ def build_cdc_postgres_init_db_volume(settings: Any) -> dict[str, dict[str, str]
 # are changed by sentry configoptions.
 OPTIONS_AUTOMATOR_SLACK_WEBHOOK_URL: Optional[str] = None

+# This setting is specific to the options automator. It gets the current region or
+# customer in the case of single tenant.
+SENTRY_REGION_OR_CUSTOMER: Optional[str] = (


Two issues here.

I am a little skeptic that we do not already have a unified way to identiy ST customers or regions. I'd expect Hybrid Cloud would have already dealt with this problem. Mind checking in with them if there is arleady a library somewhere that returns the tenant/region ?

If such api to get the region/tenant does not exist, please do not register this unified value as a setting.
By registering something as a setting you are stating that the value has a default but can be customized and overridden. Which is not what you want. What you want is that sentry_region_or_customer is always either sentry_region or customer_id or None. So you never want this to be overridden to something else.
Thus please move this as a variable or constant inside the presenter that needs it.

Other argument against a dedicated setting. Thiw would add logic into the settings file. This would have unintended consequences that you do not want. It implies the setting file must always be a python file or be able to store arbitrary logic, which makes it less like a setting file and make it hard to move to something like a json/yaml file.

fpacifici · 2023-08-14T04:57:29Z

src/sentry/runner/commands/presenters/slackpresenter.py

+        region = settings.SENTRY_REGION
+
+        if not region:
+            region = os.environ.get("CUSTOMER_ID")


Please do not rely on the env variable directly in application code.
Add a setting that defaults to os.environ.get("CUSTOMER_ID") just like settings.SENTRY_REGION
That's how the application has access to env vars.

The app does not need to know what is set by overriding a setting file or by providing an environment variables. This is a choice of who runs sentry.

We use the settings module as an abstraction so that the application does not have to take both settings and environment variable into account every time it needs a setting.

fpacifici · 2023-08-15T16:29:54Z

src/sentry/runner/commands/presenters/slackpresenter.py

@@ -66,7 +66,12 @@ def is_slack_enabled():
            raise

    def flush(self) -> None:
+        region: Optional[str] = settings.SENTRY_REGION
+        if not region:
+            region = settings.CUSTOMER_ID


What is this is None as well ? Would the webhook break if you pass None ?

Webhook will not break, it will default to sending the message without region defined. I can reconfigure this behavior in eng-pipes as needed, but I think thats fine.

kneeyo1 added 2 commits August 11, 2023 10:16

added region

17977d4

added region

354a34a

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 11, 2023

added comments explaining the region

a934413

vercel bot deployed to Preview August 11, 2023 15:22 View deployment

added None

2d6fbf5

vercel bot deployed to Preview August 11, 2023 15:34 View deployment

kneeyo1 commented Aug 11, 2023

View reviewed changes

removing safe_dump

0591ec3

vercel bot deployed to Preview August 11, 2023 16:10 View deployment

fix testcase

868ce3c

vercel bot deployed to Preview August 11, 2023 17:03 View deployment

fpacifici requested changes Aug 11, 2023

View reviewed changes

using customer id

abd8349

vercel bot deployed to Preview August 11, 2023 19:38 View deployment

fpacifici reviewed Aug 14, 2023

View reviewed changes

addressed comments

d9f6c41

vercel bot deployed to Preview August 14, 2023 13:47 View deployment

kneeyo1 requested a review from fpacifici August 15, 2023 13:28

fpacifici approved these changes Aug 15, 2023

View reviewed changes

kneeyo1 merged commit 722aee0 into master Aug 15, 2023

kneeyo1 deleted the feat/opt-automator-add-region branch August 15, 2023 17:08

github-actions bot locked and limited conversation to collaborators Aug 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Feat/opt automator add region #54631

Feat/opt automator add region #54631

Uh oh!

kneeyo1 commented Aug 11, 2023 •

edited

Loading

Uh oh!

kneeyo1 Aug 11, 2023

Uh oh!

fpacifici Aug 11, 2023

Uh oh!

codecov bot commented Aug 11, 2023 •

edited

Loading

Uh oh!

fpacifici commented Aug 11, 2023

Uh oh!

fpacifici Aug 11, 2023

Uh oh!

fpacifici Aug 11, 2023

Uh oh!

kneeyo1 Aug 11, 2023

Uh oh!

fpacifici Aug 11, 2023

Uh oh!

fpacifici Aug 14, 2023

Uh oh!

fpacifici Aug 15, 2023

Uh oh!

kneeyo1 Aug 15, 2023

Uh oh!

Uh oh!

Uh oh!

Feat/opt automator add region #54631

Feat/opt automator add region #54631

Uh oh!

Conversation

kneeyo1 commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fpacifici commented Aug 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kneeyo1 commented Aug 11, 2023 •

edited

Loading

codecov bot commented Aug 11, 2023 •

edited

Loading