-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sql: add telemetry for schema changer mode #105108
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
fdf78a9
to
141747a
Compare
141747a
to
e48f359
Compare
Now the check uses the statement tag to determine which operation is running. I took the opportunity to make sure the statement tags matched with the ones Postgres returns. Release note: None
e48f359
to
4a7c315
Compare
pkg/sql/opaque.go
Outdated
@@ -56,6 +58,9 @@ func buildOpaque( | |||
plan, err = p.SchemaChange(ctx, stmt) | |||
if err != nil { | |||
return nil, err | |||
} else if plan != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create function has a similar trick since it's not opaque and have to go through optbuilder at the moment:
cockroach/pkg/sql/opt_exec_factory.go
Line 1894 in f2fa926
if plan != nil { |
Maybe better to do the counting in
p.SchemaChange
method, or I'm good with adding an extra counting there as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for pointing that out! i added tests for it, and moved the logic to p.SchemaChange
i noticed a few more places with missing telemetry, so made a followup PR: #105482
This adds feature counter telemetry as well as log based telemetry to indicate which version of the schema changer was used. Release note: None
4a7c315
to
6192117
Compare
bors r+ |
Build failed: |
bors r+ |
Build succeeded: |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from a34ef58 to blathers/backport-release-22.2-105108: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 22.2.x failed. See errors above. error creating merge commit from 6192117 to blathers/backport-release-23.1-105108: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
sql: use featureflag check for new schema changer
Now the check uses the statement tag to determine which operation is
running.
I took the opportunity to make sure the statement tags matched with the
ones Postgres returns.
sql: add telemetry for schema changer mode
fixes #99386
This adds feature counter telemetry as well as log based telemetry to
indicate which version of the schema changer was used.
The telemetry keys are:
sql.schema.schema_changer_mode.legacy
sql.schema.schema_changer_mode.declarative
The new field in the log-based telemetry is:
SchemaChangerMode
Release note: None