sql: add metric for schema change job user/database errors #68252

fqazi · 2021-07-29T20:56:23Z

Previously, we had no way of tracking the reason
a schema change job failed in our metrics. This was
inadequate because we had no way of knowing why
schema jobs failed. To address this, this patch
will add metrics for tracking failures due to constraint
violations and database errors.

Release note (sql change): Add a new metrics to track schema job failure
(sql.schema_changer.errors.all,
sql.schema_changer.errors.constraint_violation,
sql.schema_changer.errors.uncategorized), errors inside the
crdb_internal.feature_usage table.

cockroach-teamcity · 2021-07-29T21:00:21Z

This change is

postamar

Just a few comments from me.

postamar · 2021-08-02T13:44:36Z

pkg/sql/logictest/testdata/logic_test/alter_table

+query I
+SELECT count(usage_count)
+  FROM crdb_internal.feature_usage
+ WHERE feature_name = 'sql.schema_changer.errors.constraint_violation' and usage_count > 1;


I find this query quite confusing to be honest. What is the purpose and meaning of the usage_count > 1 clause? Shouldn't this be a sum instead of a count?

postamar · 2021-08-02T13:54:51Z

pkg/sql/schema_changer.go

+				} else {
+					telemetry.Inc(sc.metrics.UncategorizedErrors)
+				}
+				telemetry.Inc(sc.metrics.AllErrors)


There's nothing wrong with this AllErrors counter per se, but since it can be trivially inferred by summing the other two above I'd be inclined to remove it.

When running analytics queries on the counters it's nice to be able to aggregate on the parent key. In CRDB this means SELECT ..., sum(usage_count) FROM crdb_internal.feature_usage WHERE feature_name LIKE 'sql.schema_changer.errors.%' AND ... GROUP BY ... and in Snowflake it's I don't know what but I'd wager a guess that it's nicer for the clients of these counters to not have to special-case AllErrors.

postamar · 2021-08-02T14:00:16Z

pkg/sql/schema_changer.go

+		pgCode == pgcode.ForeignKeyViolation ||
+		pgCode == pgcode.NotNullViolation ||
+		pgCode == pgcode.IntegrityConstraintViolation
+}


Assuming this is correct (I honestly don't know) this might not age so well if we need to cover new failure codes. Have you considered making the pgcode itself part of the telemetry counter key? That way you wouldn't have to worry about this. I believe it'd be OK to shift the burden of interpreting these to the client. cc @vy-ton

pgcode itself part of the telemetry counter key
Do you mean a counter for every time a pgcode is returned? Could that be noisy and include instances when these specific codes are encountered in non-schema changes?

When we have the new telemetry logging infra, I think tracking pgcode errors would be a great thing to add.

fqazi

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

pkg/sql/schema_changer.go, line 151 at r1 (raw file):

Previously, vy-ton (Vy Ton) wrote…

pgcode itself part of the telemetry counter key
Do you mean a counter for every time a pgcode is returned? Could that be noisy and include instances when these specific codes are encountered in non-schema changes?

When we have the new telemetry logging infra, I think tracking pgcode errors would be a great thing to add.

Does that mean right now we are okay with this approach, due to limitations of existing infrasturcture? I do like the idea of shifting the burden on the clients and just adding this in the key @postamar @vy-ton

pkg/sql/schema_changer.go, line 2135 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

There's nothing wrong with this AllErrors counter per se, but since it can be trivially inferred by summing the other two above I'd be inclined to remove it.

When running analytics queries on the counters it's nice to be able to aggregate on the parent key. In CRDB this means SELECT ..., sum(usage_count) FROM crdb_internal.feature_usage WHERE feature_name LIKE 'sql.schema_changer.errors.%' AND ... GROUP BY ... and in Snowflake it's I don't know what but I'd wager a guess that it's nicer for the clients of these counters to not have to special-case AllErrors.

Good point, I'll drop the allErrors counter, since it's trivial to compute.

pkg/sql/logictest/testdata/logic_test/alter_table, line 1848 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

I find this query quite confusing to be honest. What is the purpose and meaning of the usage_count > 1 clause? Shouldn't this be a sum instead of a count?

This is more of a sanity check at the end of the test depending on the mode we are running for the logic test, there may be either N or more failures. I'll make this saner by having it greater than N. The count is just mean to give a binary indicator that a greater or equal number was seen.

fqazi

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

pkg/sql/logictest/testdata/logic_test/alter_table, line 1848 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

This is more of a sanity check at the end of the test depending on the mode we are running for the logic test, there may be either N or more failures. I'll make this saner by having it greater than N. The count is just mean to give a binary indicator that a greater or equal number was seen.

*Sorry its because the errors sum across the different modes

postamar · 2021-08-12T14:53:44Z

LGTM

fqazi · 2021-08-12T14:55:00Z

@postamar TFTR!

Fixes: cockroachdb#68111 Previously, we had no way of tracking the reason a schema change job failed in our metrics. This was inadequate because we had no way of knowing why schema jobs failed. To address this, this patch will add metrics for tracking failures due to constraint violations and database errors. Release note (sql change): Add a new metrics to track schema job failure (sql.schema_changer.errors.all, sql.schema_changer.errors.constraint_violation, sql.schema_changer.errors.uncategorized), errors inside the crdb_internal.feature_usage table.

fqazi · 2021-08-12T19:38:45Z

bors r=postamar

craig · 2021-08-12T20:54:14Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2021-08-12T22:49:14Z

Build failed:

GitHub CI (Cockroach)

fqazi · 2021-08-13T01:40:32Z

bors r=postamar

craig · 2021-08-13T03:03:07Z

Build succeeded:

GitHub CI (Cockroach)

fqazi force-pushed the schemaChangeFailErrorType branch from 65ca358 to 4043b8b Compare July 30, 2021 14:34

fqazi requested a review from a team July 30, 2021 14:34

fqazi marked this pull request as ready for review July 30, 2021 14:34

fqazi force-pushed the schemaChangeFailErrorType branch from 4043b8b to 26f7760 Compare July 30, 2021 18:28

postamar reviewed Aug 2, 2021

View reviewed changes

fqazi commented Aug 4, 2021

View reviewed changes

fqazi force-pushed the schemaChangeFailErrorType branch from 26f7760 to 881a77f Compare August 4, 2021 02:45

postamar self-requested a review August 12, 2021 14:53

fqazi force-pushed the schemaChangeFailErrorType branch from 881a77f to 3375d87 Compare August 12, 2021 19:38

craig bot merged commit 3928f19 into cockroachdb:master Aug 13, 2021

jseldess mentioned this pull request Sep 8, 2021

sql: add metric for schema change job user/database errors cockroachdb/docs#11422

Closed

fqazi mentioned this pull request Sep 23, 2021

release-21.1: sql: add metric for schema change job user/database errors #70621

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add metric for schema change job user/database errors #68252

sql: add metric for schema change job user/database errors #68252

fqazi commented Jul 29, 2021 •

edited

Loading

cockroach-teamcity commented Jul 29, 2021

postamar left a comment

postamar Aug 2, 2021

postamar Aug 2, 2021

postamar Aug 2, 2021

vy-ton Aug 2, 2021

fqazi left a comment

fqazi left a comment

postamar commented Aug 12, 2021

fqazi commented Aug 12, 2021

fqazi commented Aug 12, 2021

craig bot commented Aug 12, 2021

craig bot commented Aug 12, 2021

fqazi commented Aug 13, 2021

craig bot commented Aug 13, 2021

sql: add metric for schema change job user/database errors #68252

sql: add metric for schema change job user/database errors #68252

Conversation

fqazi commented Jul 29, 2021 • edited Loading

cockroach-teamcity commented Jul 29, 2021

postamar left a comment

Choose a reason for hiding this comment

postamar Aug 2, 2021

Choose a reason for hiding this comment

postamar Aug 2, 2021

Choose a reason for hiding this comment

postamar Aug 2, 2021

Choose a reason for hiding this comment

vy-ton Aug 2, 2021

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

postamar commented Aug 12, 2021

fqazi commented Aug 12, 2021

fqazi commented Aug 12, 2021

craig bot commented Aug 12, 2021

craig bot commented Aug 12, 2021

fqazi commented Aug 13, 2021

craig bot commented Aug 13, 2021

fqazi commented Jul 29, 2021 •

edited

Loading