Send pulse triggers should respect report timezone #42502

qnkhuat · 2024-05-10T11:09:02Z

We use the cron job to send pulse triggers, but we need to ensure it's triggered in the report-timezone; this PR ensures we do that and also makes changing the report-timezone update all trigger's timezone.

Reported in: https://metaboat.slack.com/archives/CKZEMT1MJ/p1715331817582219

replay-io · 2024-05-10T11:26:10Z

Status	Complete ↗︎
Commit	`757093e`
Results	⚠️ 7 Flaky ✅ 2476 Passed

calherries · 2024-05-10T12:20:17Z

src/metabase/task.clj

@@ -265,7 +265,7 @@
   :priority           (.getPriority trigger)
   :start-time         (.getStartTime trigger)
   :may-fire-again?    (.mayFireAgain trigger)
-   :data               (.getJobDataMap trigger)})
+   :data               (into {} (.getJobDataMap trigger))})


Why did you need to do this?

we need this in tests because we can't compare JobDataMap with clojure map

qnkhuat · 2024-05-10T12:21:57Z

src/metabase/task/send_pulses.clj

@@ -75,6 +87,7 @@
   (triggers/with-schedule
     (cron/schedule
      (cron/cron-schedule (u.cron/schedule-map->cron-string schedule-map))
+      (cron/in-time-zone (TimeZone/getTimeZone ^String timezone))


this is the fix

calherries · 2024-05-10T13:33:20Z

Why should updating the report time zone change the trigger of every pulse? For example, if I schedule an alert at midnight when the report time zone is time zone A, then I change the report time zone to time zone B, I would still expect the alert to fire at midnight in time zone A. Maybe that's how it worked previously, but that seems very surprising.

calherries · 2024-05-10T16:44:13Z

src/metabase/task/send_pulses.clj

+(defn- send-trigger-timezone
+  []
+  (or (driver/report-timezone)
+      (qp.timezone/system-timezone-id)
+      "UTC"))


This feels like it could be DRY'd up with other parts of the codebase outside of pulses. It doesn't feel right to have a notion of the trigger time zone specific to send pulses task.

Also, before we were just using (driver/report-timezone) without (qp.timezone/system-timezone-id), can you explain the change?

it's the fallback timezone id we used in QP so I think we should apply it for pulse too:

metabase/src/metabase/query_processor/timezone.clj

Line 90 in 907da2d

(if use-report-timezone-id-if-unsupported?

It's also used in other tasks like persist refresh : https://github.com/metabase/metabase/blob/send-pulse-trigger-respect-timezone/src/metabase/task/persist_refresh.clj#L286

Right, I'm suggesting to DRY this up then. Otherwise they can get out of sync. Instead of duplicating this logic again, update-send-pulse-triggers-timezone!, and the cron-schedule function should use the same function that contains the logic for getting the reporting time zone ID. Why not use results-timezone-id from the QP code?

results-timezone-id requires a DB. In case of pulse a dashboard can contain questions from more than 1 DB.

Wdyt if we move this to metabase.task or a new metabase.task.util?

I don't mind where it goes too much. But I'm seeing a couple more places this could be DRYed up too:

metabase/src/metabase/email/messages.clj

Lines 577 to 579 in e6f4795

(defn- schedule-timezone

[]

(or (driver/report-timezone) "UTC"))

metabase/src/metabase/task/index_values.clj

Lines 82 to 84 in e6f4795

(cron/in-time-zone (TimeZone/getTimeZone (or (driver/report-timezone)

(qp.timezone/system-timezone-id)

"UTC")))

qnkhuat · 2024-05-13T02:31:08Z

Why should updating the report time zone change the trigger of every pulse?

answered in https://metaboat.slack.com/archives/C0641E4PB9B/p1715349254844079

crisptrutski

LGTM but will let Cal have final say

crisptrutski · 2024-05-14T09:58:28Z

src/metabase/driver.clj

+  (classloader/require 'metabase.task.send-pulses)
+  ((resolve 'metabase.task.send-pulses/update-send-pulse-triggers-timezone!)))


It'll be nice to replace this sneak hidden cross-module dependency with an event system - maybe in scope for the notifications refactor?

yes, it's one of my annoyance as well!

…zone

calherries

Works for me as long as the duplication of logic around the reporting time zone is resolved. That could be left to another PR though, because it looks like an existing issue

…n up send-pulse triggers (#42316) * Add a job to init send pulse trigger only once * Send pulse triggers should respect report timezone (#42502)

Send pulse triggers should respect report timezone

7c20853

qnkhuat requested a review from camsaul as a code owner May 10, 2024 11:09

metabase-bot bot assigned qnkhuat May 10, 2024

metabase-bot bot added the .Team/BackendComponents also known as BEC label May 10, 2024

polish

e76e819

fix tests failed to compile

a23e0c1

qnkhuat requested a review from a team May 10, 2024 12:17

calherries reviewed May 10, 2024

View reviewed changes

qnkhuat commented May 10, 2024

View reviewed changes

calherries reviewed May 10, 2024

View reviewed changes

test fixes

907da2d

qnkhuat requested a review from a team May 13, 2024 03:01

test fixes

6ef0844

crisptrutski reviewed May 14, 2024

View reviewed changes

qnkhuat requested a review from calherries May 15, 2024 02:40

Merge branch 'init-trigger-once' into send-pulse-trigger-respect-time…

78e24b1

…zone

calherries approved these changes May 15, 2024

View reviewed changes

fix tests failed to compile

757093e

qnkhuat merged commit d1a25be into init-trigger-once May 15, 2024
77 of 78 checks passed

qnkhuat deleted the send-pulse-trigger-respect-timezone branch May 15, 2024 04:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send pulse triggers should respect report timezone #42502

Send pulse triggers should respect report timezone #42502

qnkhuat commented May 10, 2024 •

edited

replay-io bot commented May 10, 2024 •

edited

calherries May 10, 2024

qnkhuat May 10, 2024

qnkhuat May 10, 2024

calherries commented May 10, 2024

calherries May 10, 2024

qnkhuat May 13, 2024

calherries May 15, 2024 •

edited

qnkhuat May 15, 2024 •

edited

calherries May 15, 2024

qnkhuat commented May 13, 2024 •

edited

crisptrutski left a comment

crisptrutski May 14, 2024

qnkhuat May 15, 2024

calherries left a comment •

edited

	(defn- schedule-timezone
	[]
	(or (driver/report-timezone) "UTC"))

	(cron/in-time-zone (TimeZone/getTimeZone (or (driver/report-timezone)
	(qp.timezone/system-timezone-id)
	"UTC")))

		(classloader/require 'metabase.task.send-pulses)
		((resolve 'metabase.task.send-pulses/update-send-pulse-triggers-timezone!)))

Send pulse triggers should respect report timezone #42502

Send pulse triggers should respect report timezone #42502

Conversation

qnkhuat commented May 10, 2024 • edited

replay-io bot commented May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calherries commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calherries May 15, 2024 • edited

Choose a reason for hiding this comment

qnkhuat May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qnkhuat commented May 13, 2024 • edited

crisptrutski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calherries left a comment • edited

Choose a reason for hiding this comment

qnkhuat commented May 10, 2024 •

edited

replay-io bot commented May 10, 2024 •

edited

calherries May 15, 2024 •

edited

qnkhuat May 15, 2024 •

edited

qnkhuat commented May 13, 2024 •

edited

calherries left a comment •

edited