ui: introduce foundations to support configurable timezone display

[zachlite]

This commit adds the foundational dependencies and components needed to display timestamps in DB console in a user's preferred timezone.

Introduced:

• A dependency on moment-timezone in DB console and Cluster UI.
• A cluster setting: ui.display_timezone.
• A timezone context with a default value of "UTC". This enables backwards-compatability for cluster-ui components. No changes in cloud are needed, and cluster-ui components will continue to display timestamps in UTC.
• A context provider at the top of DB console's component tree.

Part of: #40419
Epic: https://cockroachlabs.atlassian.net/browse/CRDB-24406
Release note: None

[cockroach-teamcity]

This change is 

[koorosh]

Reviewed 150 of 150 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @zachlite)

a discussion (no related file):
LGTM in general! I like the idea of using context to propagate selected timezone!

---

---

pkg/ui/workspaces/cluster-ui/src/contexts/timezoneContext.tsx line 14 at r1 (raw file):

import { createContext, useContext } from "react";

export const TimezoneContext = createContext<string>("UTC");

Do we need to set default timezone for moment as described here: https://momentjs.com/timezone/docs/#/using-timezones/default-timezone/

moment.tz.setDefault("UTC");

---

pkg/ui/workspaces/db-console/src/contexts/timezoneProvider.tsx line 24 at r1 (raw file):

  const dispatch = useDispatch();
  const settings = useSelector(selectClusterSettings);
  useEffect(() => {

It is safe to dispatch refreshSettings() action on initial component load without validation for null.
refreshSettings() will be ignored if it is called within some interval and cached value returned instead. And calling without validation would guarantee that requested value is not invalidated.

[j82w]

-- commits line 17 at r1:
Can you add a loom video showing the new functionality? Also please add a release note since this a new feature for users.

[j82w]

pkg/ui/workspaces/db-console/package.json line 38 at r1 (raw file):

    "mini-create-react-context": "^0.3.2",
    "moment": "^2.19.3",
    "moment-timezone": "^0.5.42",

Do new packages need to go through any approval process to verify the license?

[zachlite]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w and @koorosh)

a discussion (no related file):

Previously, koorosh (Andrii Vorobiov) wrote…

LGTM in general! I like the idea of using context to propagate selected timezone!

👍

---

---

-- commits line 17 at r1:

Previously, j82w (Jake) wrote…

Can you add a loom video showing the new functionality? Also please add a release note since this a new feature for users.

There's no new functionality here. This is only laying the groundwork for user-facing changes.

---

pkg/ui/workspaces/cluster-ui/src/contexts/timezoneContext.tsx line 14 at r1 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

Do we need to set default timezone for moment as described here: https://momentjs.com/timezone/docs/#/using-timezones/default-timezone/

moment.tz.setDefault("UTC");

I wish it could be this easy! But there's so many places where we call moment.utc that I think it would be easier to leave moment.tz.setDefault out of the equation. Otherwise, we'd have half the codebase magically controlled by setDefault, and half not controlled. I think that would be confusing.

---

pkg/ui/workspaces/db-console/package.json line 38 at r1 (raw file):

Previously, j82w (Jake) wrote…

Do new packages need to go through any approval process to verify the license?

moment-timezone is MIT licensed, and the license is distributed with the downloaded package, so I think we're good!

---

pkg/ui/workspaces/db-console/src/contexts/timezoneProvider.tsx line 24 at r1 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

It is safe to dispatch refreshSettings() action on initial component load without validation for null.
refreshSettings() will be ignored if it is called within some interval and cached value returned instead. And calling without validation would guarantee that requested value is not invalidated.

Cool! Updated.

[koorosh]

Reviewed 1 of 1 files at r2, 11 of 11 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w, @xinhaoz, and @zachlite)

---

pkg/ui/ui.go line 41 at r3 (raw file):

)

var _ = settings.RegisterEnumSetting(

How this setting can be changed to use non-default value?

---

pkg/ui/workspaces/cluster-ui/src/contexts/timezoneContext.tsx line 14 at r1 (raw file):

Previously, zachlite wrote…

I wish it could be this easy! But there's so many places where we call moment.utc that I think it would be easier to leave moment.tz.setDefault out of the equation. Otherwise, we'd have half the codebase magically controlled by setDefault, and half not controlled. I think that would be confusing.

Makes sense!

[maryliag]

Reviewed 138 of 150 files at r1, 1 of 1 files at r2, 11 of 11 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w, @koorosh, @xinhaoz, and @zachlite)

a discussion (no related file):

Previously, zachlite wrote…

👍

Nice foundation! Left a few nits

---

---

pkg/ui/ui.go line 38 at r3 (raw file):

const (
	utc int64 = iota
	americaNewYork

Is there a requirement to use NY? Asking because several other places use this timezone and is common to called EST time, so maybe you should go with that.

---

pkg/ui/workspaces/cluster-ui/src/util/format.ts line 195 at r3 (raw file):

export const DATE_FORMAT_24_UTC = "MMM DD, YYYY [at] H:mm UTC";
export const DATE_WITH_SECONDS_FORMAT_24_UTC = "MMM DD, YYYY [at] H:mm:ss UTC";
export const DATE_WITH_SECONDS_FORMAT_24_TZ = "MMM DD, YYYY [at] H:mm:ss z";

should you also be creating one for DATE_FORMAT_24_TZ (without the seconds)?
and then your function below can select with or without seconds

---

pkg/ui/workspaces/db-console/src/contexts/timezoneProvider.tsx line 21 at r3 (raw file):

export const TimezoneProvider = (props: any) => {
  // Refresh cluster settings if needed

very nit: period at end of comments

---

pkg/ui/workspaces/db-console/src/redux/clusterSettings/clusterSettings.selectors.ts line 30 at r3 (raw file):

    }
    const setting = settings["ui.display_timezone"];
    return setting ? setting.value : "UTC";

you can combine these two lines as return settings["ui.display_timezone"]?.value || "UTC"

[zachlite]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w, @koorosh, @maryliag, and @xinhaoz)

---

pkg/ui/ui.go line 38 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

Is there a requirement to use NY? Asking because several other places use this timezone and is common to called EST time, so maybe you should go with that.

America/New_York gives the actual local time regardless of whether daylight savings time is being observed.

Right now, NYC is observing EDT (eastern daylight time)

And I figured when we want to support more timezones, we can add them to the list and ensure that the timezone library includes the proper data.

---

pkg/ui/ui.go line 41 at r3 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

How this setting can be changed to use non-default value?

UTC: SET CLUSTER SETTING ui.display_timezone = 0;
America/New_York: SET CLUSTER SETTING ui.display_timezone = 1;

---

pkg/ui/workspaces/cluster-ui/src/util/format.ts line 195 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

should you also be creating one for DATE_FORMAT_24_TZ (without the seconds)?
and then your function below can select with or without seconds

Yes. I clean all of these up in my forthcoming PR where I add the user-facing changes.
I wanted to keep the diff as small as possible here.

---

pkg/ui/workspaces/db-console/src/contexts/timezoneProvider.tsx line 21 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

very nit: period at end of comments

Will fix.

---

pkg/ui/workspaces/db-console/src/redux/clusterSettings/clusterSettings.selectors.ts line 30 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

you can combine these two lines as return settings["ui.display_timezone"]?.value || "UTC"

Will fix.

[maryliag]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @j82w, @koorosh, and @xinhaoz)

[maryliag]

Looking great!
As long as tests are passing

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @j82w, @koorosh, and @xinhaoz)

[sjbarag]

Watch out! Adding moment-timezone adds ~950KB to the total bundle size of cluster-ui alone: https://bundlephobia.com/package/[email protected] (and confirmed with both webpack's stdout warning and du -k dist/js/main.js before and after this change).

We'll likely want to pull in moment-timezone-data-webpack-plugin to limit the number of years of timezone data we include in the bundle: https://momentjs.com/timezone/docs/#/use-it/webpack/ I can't tell how many years are included by default, but it's quite likely more than 60 years considering that the 1970-2030 time range is called out explicitly as "not the full data file".

[zachlite]

@sjbarag thanks for catching that! I've added the plugin, and the bundle is pruned of extraneous timezone data.

[sjbarag]

Your webpack changes look good! Thanks for vendoring the new dependencies too :) Just one question about the number of years included!

[sjbarag]

Thanks for the quick fixes, @zachlite!

[zachlite]

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)