Merge cockroachdb#107718 cockroachdb#107822

craig[bot] · fantapop · yuzefovich · craig[bot] · commit b27094b0ded0 · 2023-07-31T16:55:04.000Z
107718: ui: add an overridable logger r=fantapop a=fantapop A setLogger and getLogger method are added to cluster-ui so that the cockroach cloud console can use them to pass a custom logger which embellishes each log invocation with the cluster version and cluster id. CC-24092 Release Note: none 107822: geo: fix panic in GeometryToEncodedPolyline for GeometryCollection r=yuzefovich a=yuzefovich This is not supported in Postgres and results in an error, but we previously would crash. Now fixed. Fixes: cockroachdb#107773. Release note (bug fix): CockroachDB would previously crash when evaluating `st_asencodedpolyline` builtin on 'GeometryCollection' geometry type. The bug was introduced before 22.1 and is now fixed. Co-authored-by: Christopher Fitzner <fitzner@cockroachlabs.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
diff --git a/pkg/geo/parse.go b/pkg/geo/parse.go
@@ -264,6 +264,9 @@ func GeometryToEncodedPolyline(g Geometry, p int) (string, error) {
 	if gt.SRID() != 4326 {
 		return "", pgerror.Newf(pgcode.InvalidParameterValue, "only SRID 4326 is supported")
 	}
+	if _, ok := gt.(*geom.GeometryCollection); ok {
+		return "", pgerror.Newf(pgcode.InvalidParameterValue, "'GeometryCollection' geometry type not supported")
+	}
 
 	return encodePolylinePoints(gt.FlatCoords(), p), nil
 }
diff --git a/pkg/sql/logictest/testdata/logic_test/geospatial_regression b/pkg/sql/logictest/testdata/logic_test/geospatial_regression
@@ -7,3 +7,5 @@ CREATE TABLE regression_81609 (
 statement error cannot index a geometry with NaN coordinates
 INSERT INTO regression_81609 SELECT st_rotatex('0105000000060000000102000000020000002CF565DB1790F041C2509EC57231F2411C1E119D614AE7C166AFF2D39B030142010200000003000000809745B2972CF84160399714F897E241E0D03302F1C6D6410EFC89BE9FE20042F83C7FD5C4ACD6411854CBFDFF42DE41010200000004000000C4A695BE1FC2FD411CE9BCE66C42D8C142F59F55FD7800424C0D1238165FE041A07574B08DC0D94170D034F5A969EB41D843A641FF49E741B6F0864FF80E0242010200000004000000781D60294113F2412C338441624C02C2100451CE196BED41525C0281389BEEC11430818C69A2EF415C09A4FD752D024200E4B02D00CB9341C047BBFE74DDE44101020000000400000040B0B6B74C96A0C162D23CFAACB8E7C11CD943F294E9E5C187C18248646102C2709F7E29F32AC4C160D1A56EC799FAC1607737DC4662E941ADC9125B944FF6C1010200000004000000F097AF975517F441714AF1717A16F0C15442BA391710F841BEBE416B6780F241E01C9C5F7602B6413CC620A0C9C3E14140E714D68300E2C1A0C4038270B4BAC1':::GEOMETRY::GEOMETRY, '-Inf':::FLOAT8)::GEOMETRY;
 
+statement error 'GeometryCollection' geometry type not supported
+SELECT st_asencodedpolyline(geomfromewkb(('01070000A0E61000000300000001060000A0E6100000040000000103000080010000000B0000006F9AF3F7D81863C057397A0B3ACD42C006ED79D13D2DF24130E1CBF62A4A45C01B6DC0D279F452C022F8C672D2C1F9C140DB9D23050141C0B53FBA6E396656C0C0A1842E2B53EC413A8DC94C568D61401BEF0352091651C05C681DE175D2FD41A048C859DF7C5440C00E327AC97FF63F68BBA8144D0EF8416E6042F340DD6040805791CE56602740DC6C3922676401C29C6D4009B1334E40625E855D7A7E5140D4241F7D86D2D5C14B1E4BCA077A58C07806BF9D4D084D4074E74BE4DCAA00C2C11D020045A051C0D81F8E3DD0AE3740B8315FF7F63AF4C1BAD97494BA9860C07411EFAF0A6C3D4038E26DE7A194F4C16F9AF3F7D81863C057397A0B3ACD42C006ED79D13D2DF2410103000080010000000B000000610D90AE22A55FC0B8265672F7562940CE3E91EB7A320042CC96C05617B649C085229B1A09E153C0D84FA7998764EB4190FEB5A7C71B2DC03EBBCB6B588F4DC02891E1ED4A7DE6417C0F7BBC14C75840E08D4D210E2F21C04180E453EDEDFFC112E57185B2435240F0D20FC2D7C11640F8BF2E3AB98DD2C1D0A9A94D6EA14140FCABD1E5BF074B400010C2F75AA16F4166BC301BE6FB55C002F9F182DE3D53400038B551BC0495C199463681FA6E5CC0184D3E0AE179554030323F0F2525F4415791FBE04FEA61C0361E8733C77A5240C84AEBEAD466EEC1D5012A75E04F63C0A8BF403F623D4240182CFEFA9071E0C1610D90AE22A55FC0B8265672F7562940CE3E91EB7A3200420103000080010000000C000000004CADA6B302E0BF3EE5FA281ACE3BC0E051ABD6580901429B65A22CCBDC53C003DEBC27260E4AC07E45691D3E7AF541408E6FAB9A1008C016E33E2631FD40C0EEF80F0FF3E8F341A8C5EAA8B0C836409E85B8193CE952C0F4448FFF9B31F24190F9A9FCD2AC604004F99C51ED4356C0903A1F652902D4C1C01B737BD389554080315427550738C05C51AE075475F64124F6F92382235C40148AB5FED33D38C048960D3AC6D7E8412C2E310EABA5514058EA10C427562EC0A82D4D1A69D7FE41621FC77070C26540FE84B78FE5BE51400962385E53FAFEC1483899F770B14DC020BD4BBBAB604D401C08306EA143E84140337962764325C0002F089A1DF3F83FAC9B66F32C2BE841004CADA6B302E0BF3EE5FA281ACE3BC0E051ABD65809014201030000800100000006000000D03FF366A2C95C40E59BFCECDD7553C0D028B038BB63D041A40AEF5CB0195840006FB928107F084020CA0A39A4AED34100F09D4126336140E8AC65DAC4954C4024406D4040D1EE41F04B3F3DA6C75740106594C0ADCE4440A498B6372039FD4155D08A1E777452C084CC004170E949409C87DF53EB9AFDC1D03FF366A2C95C40E59BFCECDD7553C0D028B038BB63D04101020000A0E61000000200000074F55D784E5F50408F7DE14351CD41C0FCCCE7B1B166F541583C392FCD7554409022EAF3EFCC39C03CDE1C0E29A7EB4101040000A0E6100000060000000101000080E8801DFC8A2130C04401B4AE182F50C0BC13340EB7E2E1C1010100008010DC3F808D824D406C9927CDAE5144409A464D93BBD7F94101010000806F0682A50FE253C030E1964D47F74BC00E752ECB60B0F441010100008014F5242FC2C444C01257DB8E51FA51404449E2FB7D2AFFC1010100008069945BA1FD2E50C0501CA74B7A661C40CB83CB9A4894F1C10101000080C0BBBC345A4B51C0E08F6EBB2F250E400681A78F4FFCF741'::GEOGRAPHY)::BYTEA)::GEOMETRY);
diff --git a/pkg/ui/workspaces/cluster-ui/src/api/contentionApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/contentionApi.ts
@@ -19,6 +19,7 @@ import {
 } from "./sqlApi";
 import { ContentionDetails } from "src/insights";
 import moment from "moment-timezone";
+import { getLogger } from "../util";
 
 export type ContentionFilters = {
   waitingTxnID?: string;
@@ -62,8 +63,9 @@ export async function getContentionDetailsApi(
   if (sqlResultsAreEmpty(result)) {
     if (result.error) {
       // We don't return an error if it failed to retrieve the contention information.
-      console.error(
-        `Insights encounter an error while retrieving contention information: ${result.error}`,
+      getLogger().error(
+        "Insights encounter an error while retrieving contention information.",
+        { resultError: result.error },
       );
     }
     return formatApiResult<ContentionDetails[]>(
diff --git a/pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts
@@ -26,6 +26,7 @@ import { Format, Identifier, QualifiedIdentifier } from "./safesql";
 import moment from "moment-timezone";
 import { fromHexString, withTimeout } from "./util";
 import { cockroach } from "@cockroachlabs/crdb-protobuf-client";
+import { getLogger } from "../util";
 
 const { ZoneConfig } = cockroach.config.zonepb;
 const { ZoneConfigurationLevel } = cockroach.server.serverpb;
@@ -264,8 +265,10 @@ const getDatabaseZoneConfig: DatabaseDetailsQuery<DatabaseZoneConfigRow> = {
         );
         resp.zoneConfigResp.zone_config_level = ZoneConfigurationLevel.DATABASE;
       } catch (e) {
-        console.error(
+        getLogger().error(
           `Database Details API - encountered an error decoding zone config string: ${zoneConfigHexString}`,
+          /* additional context */ undefined,
+          e,
         );
         // Catch and assign the error if we encounter one decoding.
         resp.zoneConfigResp.error = e;
diff --git a/pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts
@@ -26,6 +26,7 @@ import { fromHexString, withTimeout } from "./util";
 import { Format, Identifier, Join, SQL } from "./safesql";
 import { cockroach } from "@cockroachlabs/crdb-protobuf-client";
 import { IndexUsageStatistic, recommendDropUnusedIndex } from "../insights";
+import { getLogger } from "../util";
 
 const { ZoneConfig } = cockroach.config.zonepb;
 const { ZoneConfigurationLevel } = cockroach.server.serverpb;
@@ -314,8 +315,10 @@ const getTableZoneConfig: TableDetailsQuery<TableZoneConfigRow> = {
         );
         resp.zoneConfigResp.zone_config_level = configLevel;
       } catch (e) {
-        console.error(
+        getLogger().error(
           `Table Details API - encountered an error decoding zone config string: ${hexString}`,
+          /* additional context */ undefined,
+          e,
         );
         // Catch and assign the error if we encounter one decoding.
         resp.zoneConfigResp.error = e;
diff --git a/pkg/ui/workspaces/cluster-ui/src/loading/loading.tsx b/pkg/ui/workspaces/cluster-ui/src/loading/loading.tsx
@@ -17,7 +17,7 @@ import {
   Spinner,
   InlineAlertIntent,
 } from "@cockroachlabs/ui-components";
-import { adminUIAccess, isForbiddenRequestError } from "src/util";
+import { adminUIAccess, getLogger, isForbiddenRequestError } from "src/util";
 import styles from "./loading.module.scss";
 import { Anchor } from "../anchor";
 
@@ -68,7 +68,14 @@ export const Loading: React.FC<LoadingProps> = props => {
   // Check for `error` before `loading`, since tests for `loading` often return
   // true even if CachedDataReducer has an error and is no longer really "loading".
   if (errors) {
-    console.error(`Error Loading ${props.page}: ${errors}`);
+    getLogger().error(
+      errors.length === 1
+        ? `Error Loading ${props.page}`
+        : `Multiple errors seen Loading ${props.page}: ${errors}`,
+      /* additional context */ undefined,
+      errors[0],
+    );
+
     // - map Error to InlineAlert props. RestrictedPermissions handled as "info" message;
     // - group errors by intend to show separate alerts per intent.
     const errorAlerts = chain(errors)
diff --git a/pkg/ui/workspaces/cluster-ui/src/store/uiConfig/uiConfig.sagas.ts b/pkg/ui/workspaces/cluster-ui/src/store/uiConfig/uiConfig.sagas.ts
@@ -14,6 +14,7 @@ import { getUserSQLRoles } from "../../api/userApi";
 import { CACHE_INVALIDATION_PERIOD, throttleWithReset } from "../utils";
 import { rootActions } from "../reducers";
 import { cockroach } from "@cockroachlabs/crdb-protobuf-client";
+import { getLogger } from "../../util";
 
 export function* refreshUserSQLRolesSaga(): any {
   yield put(actions.requestUserSQLRoles());
@@ -26,7 +27,7 @@ export function* requestUserSQLRolesSaga(): any {
     );
     yield put(actions.receivedUserSQLRoles(result.roles));
   } catch (e) {
-    console.warn(e.message);
+    getLogger().warn(e.message, /* additional context */ undefined, e);
   }
 }
 
diff --git a/pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx b/pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx
@@ -26,7 +26,7 @@ import { defaultTimeScaleOptions, findClosestTimeScale } from "./utils";
 
 import styles from "./timeScale.module.scss";
 import { TimezoneContext } from "../contexts";
-import { FormatWithTimezone } from "../util";
+import { FormatWithTimezone, getLogger } from "../util";
 
 const cx = classNames.bind(styles);
 
@@ -186,7 +186,7 @@ export const TimeScaleDropdown: React.FC<TimeScaleDropdownProps> = ({
         isMoving = true;
         break;
       default:
-        console.error("Unknown direction: ", direction);
+        getLogger().error("Unknown direction: ", direction);
     }
 
     // If the timescale extends into the future then fallback to a default
diff --git a/pkg/ui/workspaces/cluster-ui/src/util/index.ts b/pkg/ui/workspaces/cluster-ui/src/util/index.ts
@@ -17,6 +17,7 @@ export * from "./docs";
 export * from "./fixLong";
 export * from "./format";
 export * from "./formatDate";
+export * from "./logger";
 export * from "./requestError";
 export * from "./sql/summarize";
 export * from "./query";
diff --git a/pkg/ui/workspaces/cluster-ui/src/util/logger.ts b/pkg/ui/workspaces/cluster-ui/src/util/logger.ts
@@ -0,0 +1,91 @@
+// Copyright 2023 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+type JSONValue =
+  | null
+  | boolean
+  | number
+  | string
+  | JSONValue[]
+  | { [key: string]: JSONValue };
+
+interface Logger {
+  /**
+   * Logs a message with additional context and an optional error at debug
+   * level.
+   * @param msg - the string message to log
+   * @param context - additional structured context to include with the
+   * message when shipped to a remote service
+   * @param error - a possible JS Error to include with the message when
+   * shipped to a remote service. Typed as unknown for convenience but
+   * anything passed here which is not instanceof Error will not be attached.
+   */
+  debug(
+    msg: string,
+    context?: Record<string, JSONValue>,
+    error?: unknown,
+  ): void;
+  /**
+   * Logs a message with additional context and an optional error at info
+   * level.
+   * @param msg - the string message to log
+   * @param context - additional structured context to include with the
+   * message when shipped to a remote service
+   * @param error - a possible JS Error to include with the message when
+   * shipped to a remote service. Typed as unknown for convenience but
+   * anything passed here which is not instanceof Error will not be attached.
+   */
+  info(msg: string, context?: Record<string, JSONValue>, error?: unknown): void;
+  /**
+   * Logs a message with additional context and an optional error at warn
+   * level.
+   * @param msg - the string message to log
+   * @param context - additional structured context to include with the
+   * message when shipped to a remote service
+   * @param error - a possible JS Error to include with the message when
+   * shipped to a remote service. Typed as unknown for convenience but
+   * anything passed here which is not instanceof Error will not be attached.
+   */
+  warn(msg: string, context?: Record<string, JSONValue>, error?: unknown): void;
+  /**
+   * Logs a message with additional context and an optional error at error
+   * level.
+   * @param msg - the string message to log
+   * @param context - additional structured context to include with the
+   * message when shipped to a remote service
+   * @param error - a possible JS Error to include with the message when
+   * shipped to a remote service. Typed as unknown for convenience but
+   * anything passed here which is not instanceof Error will not be attached.
+   */
+  error(
+    msg: string,
+    context?: Record<string, JSONValue>,
+    error?: unknown,
+  ): void;
+}
+
+let logger: Logger = console;
+
+/**
+ * Sets the logger returned by {@link getLogger}. It was added to allow
+ * cockroach cloud to pass in a custom logger which attaches additional metadata
+ * to each call and sends errors up to datadog.
+ * @param newLogger the Logger to set
+ */
+export function setLogger(newLogger: Logger) {
+  logger = newLogger;
+}
+
+/**
+ * @returns the most recent logger set by {@link setLogger}, or console if one was never set.
+ */
+export function getLogger(): Logger {
+  return logger;
+}
diff --git a/pkg/ui/workspaces/cluster-ui/src/util/versions.ts b/pkg/ui/workspaces/cluster-ui/src/util/versions.ts
@@ -8,6 +8,8 @@
 // by the Apache License, Version 2.0, included in the file
 // licenses/APL.txt.
 
+import { getLogger } from "./logger";
+
 const versionPrefix = "10000";
 export type SemVersion = [number, number, number];
 
@@ -51,7 +53,9 @@ export function parseStringToVersion(
   if (inputString.startsWith(versionPrefix)) {
     inputString = inputString.split(versionPrefix)[1];
     if (!inputString) {
-      console.log("Unable to split version string while parsing", inputString);
+      getLogger().warn(
+        "Unable to split version string while parsing: " + inputString,
+      );
       return [0, 0, 0];
     }
   }
@@ -67,7 +71,7 @@ export function parseStringToVersion(
 
     return [parsedMajorVersion, parsedMinorVersion, parsedPatchVersion];
   } else {
-    console.log("Version string did not match", inputString);
+    getLogger().warn("Version string did not match: " + inputString);
     return [0, 0, 0];
   }
 }

Original file line number	Diff line number	Diff line change
`@@ -264,6 +264,9 @@ func GeometryToEncodedPolyline(g Geometry, p int) (string, error) {`
`264`	`264`	`if gt.SRID() != 4326 {`
`265`	`265`	`return "", pgerror.Newf(pgcode.InvalidParameterValue, "only SRID 4326 is supported")`
`266`	`266`	`}`
	`267`	`+ if _, ok := gt.(*geom.GeometryCollection); ok {`
	`268`	`+ return "", pgerror.Newf(pgcode.InvalidParameterValue, "'GeometryCollection' geometry type not supported")`
	`269`	`+ }`
`267`	`270`
`268`	`271`	`return encodePolylinePoints(gt.FlatCoords(), p), nil`
`269`	`272`	`}`
Original file line number	Diff line number	Diff line change
`@@ -14,6 +14,7 @@ import { getUserSQLRoles } from "../../api/userApi";`
`14`	`14`	`import { CACHE_INVALIDATION_PERIOD, throttleWithReset } from "../utils";`
`15`	`15`	`import { rootActions } from "../reducers";`
`16`	`16`	`import { cockroach } from "@cockroachlabs/crdb-protobuf-client";`
	`17`	`+import { getLogger } from "../../util";`
`17`	`18`
`18`	`19`	`export function* refreshUserSQLRolesSaga(): any {`
`19`	`20`	`yield put(actions.requestUserSQLRoles());`
`@@ -26,7 +27,7 @@ export function* requestUserSQLRolesSaga(): any {`
`26`	`27`	`);`
`27`	`28`	`yield put(actions.receivedUserSQLRoles(result.roles));`
`28`	`29`	`} catch (e) {`
`29`		`- console.warn(e.message);`
	`30`	`+ getLogger().warn(e.message, /* additional context */ undefined, e);`
`30`	`31`	`}`
`31`	`32`	`}`
`32`	`33`