sql: add telemetry for ALTER DATABASE PLACEMENT#69006
sql: add telemetry for ALTER DATABASE PLACEMENT#69006craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
arulajmani
left a comment
There was a problem hiding this comment.
I don't think this PR adds telemetry, it adds support for event logs. We probably want telemetry too, should we do that in a separate PR?
Separately, I think you've got a bit more to do before these event logs work correctly. I suspect if you altered a database's placement policy you wouldn't have it show up correctly in the UI. Does it? Here's an example PR to follow for the last step: https://github.com/cockroachdb/cockroach/pull/63141/files#
Reviewed 3 of 6 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @otan and @pawalt)
pkg/util/log/eventpb/ddl_events.proto, line 99 at r1 (raw file):
// The name of the database. string database_name = 3 [(gogoproto.jsontag) = ",omitempty"]; // The new survival goal
Did you mean new placement policy?
This PR adds event logging for ALTER DATABASE PLACEMENT operations. Release note: None
e65b369 to
f4216b9
Compare
pawalt
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/util/log/eventpb/ddl_events.proto, line 99 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Did you mean new placement policy?
Oops bad copypasta
arulajmani
left a comment
There was a problem hiding this comment.
The telemetry counters are missing test coverage. Check out pkg/ccl/telemetryccl/testdata/telemetry/multiregion, that's probably where we want this stuff.
Reviewed 3 of 6 files at r1, 3 of 3 files at r2, 1 of 3 files at r3.
Reviewable status:
pkg/sql/alter_database.go, line 1153 at r3 (raw file):
telemetry.Inc( sqltelemetry.AlterDatabaseSurvivalGoalCounter(
Did you mean to call AlterDatabasePlacementCounter here?
f4216b9 to
659c72d
Compare
pawalt
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/alter_database.go, line 1153 at r3 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Did you mean to call
AlterDatabasePlacementCounterhere?
Damn could have sworn I changed that. Fixed.
arulajmani
left a comment
There was a problem hiding this comment.
One testing request, but otherwise
Reviewable status:
pkg/ccl/telemetryccl/testdata/telemetry/multiregion, line 35 at r4 (raw file):
feature-usage ALTER DATABASE to_be_restricted SET PLACEMENT RESTRICTED
Let's add one for SET PLACEMENT DEFAULT as well? both create and alter database.
pkg/sql/sem/tree/data_placement.go, line 43 at r4 (raw file):
} // TelemetryName returns a representation of DataPlacement suitable for telemetry
nit: full stop.
arulajmani
left a comment
There was a problem hiding this comment.
Sorry, meant to approve.
Reviewable status:
824793c to
fc5eb79
Compare
pawalt
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/telemetryccl/testdata/telemetry/multiregion, line 35 at r4 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Let's add one for
SET PLACEMENT DEFAULTas well? both create and alter database.
Done.
arulajmani
left a comment
There was a problem hiding this comment.
I think you need to update some generated code.
Reviewed all commit messages.
Reviewable status:
This PR adds telemetry for ALTER DATABASE PLACEMENT operations. Release note: None
fc5eb79 to
d86da34
Compare
|
bors r=arulajmani |
|
Build succeeded: |
This PR adds telemetry for ALTER DATABASE PLACEMENT operations.
Release note: None
Resolves #68597