From 9f6f1e4f8c1d9475ddb2c107c5f514cfe0a48536 Mon Sep 17 00:00:00 2001 From: Adam Storm Date: Wed, 5 May 2021 08:48:47 -0400 Subject: [PATCH] sql: Add telemetry for zone configuration override Add telemetry for cases where the zone configuration of a multi-region database or table is overridden by the user. Release note: None --- .../testdata/telemetry/multiregion | 69 +++++++++++++++++++ pkg/sql/region_util.go | 24 +++++-- pkg/sql/set_zone_config.go | 1 - pkg/sql/sqltelemetry/multiregion.go | 20 ++++++ 4 files changed, 108 insertions(+), 6 deletions(-) diff --git a/pkg/ccl/telemetryccl/testdata/telemetry/multiregion b/pkg/ccl/telemetryccl/testdata/telemetry/multiregion index d4964058ca89..3ab8fca249b2 100644 --- a/pkg/ccl/telemetryccl/testdata/telemetry/multiregion +++ b/pkg/ccl/telemetryccl/testdata/telemetry/multiregion @@ -366,3 +366,72 @@ SELECT * FROM t8 WHERE a = 1 ---- sql.plan.opt.locality-optimized-search +exec +USE survive_region; +CREATE TABLE t9 (a INT PRIMARY KEY) LOCALITY REGIONAL BY ROW +---- + +feature-allowlist +sql.multiregion.zone_configuration.override.* +---- + +feature-counters +SET override_multi_region_zone_config = true; +ALTER TABLE t9 CONFIGURE ZONE USING num_replicas=10; +SET override_multi_region_zone_config = false +---- +sql.multiregion.zone_configuration.override.user 1 + +# Note that this case illustrates that once the session variable is set, we'll +# count all instances where a zone configuration is modified, even if that +# modification didn't strictly require overriding. +feature-counters +SET override_multi_region_zone_config = true; +ALTER TABLE t9 CONFIGURE ZONE USING gc.ttlseconds=10; +SET override_multi_region_zone_config = false +---- +sql.multiregion.zone_configuration.override.user 1 + +feature-counters +ALTER TABLE t9 CONFIGURE ZONE USING gc.ttlseconds=5 +---- + +feature-counters +SET override_multi_region_zone_config = true; +ALTER TABLE t9 SET LOCALITY GLOBAL; +SET override_multi_region_zone_config = false +---- +sql.multiregion.zone_configuration.override.system.table 1 + +# Ensure that no counters are set in the case where we're not overriding +feature-counters +ALTER TABLE t9 SET LOCALITY REGIONAL BY TABLE IN PRIMARY REGION; +---- + +# Note that this case illustrates that once the session variable is set, we'll +# count all instances where a table's zone configuration is modified, even if +# that modification didn't strictly require overriding. +feature-counters +SET override_multi_region_zone_config = true; +ALTER TABLE t9 SET LOCALITY GLOBAL; +SET override_multi_region_zone_config = false +---- +sql.multiregion.zone_configuration.override.system.table 1 + +feature-counters +SET override_multi_region_zone_config = true; +ALTER DATABASE d CONFIGURE ZONE USING num_replicas=10; +SET override_multi_region_zone_config = false +---- +sql.multiregion.zone_configuration.override.user 1 + +feature-counters +ALTER DATABASE d CONFIGURE ZONE USING gc.ttlseconds=5; +---- + +feature-counters +SET override_multi_region_zone_config = true; +ALTER DATABASE d ADD REGION "us-east-1"; +SET override_multi_region_zone_config = false +---- +sql.multiregion.zone_configuration.override.system.database 1 diff --git a/pkg/sql/region_util.go b/pkg/sql/region_util.go index bf96e9e30358..c06452272882 100644 --- a/pkg/sql/region_util.go +++ b/pkg/sql/region_util.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/security" + "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" @@ -29,6 +30,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/errors" "github.com/gogo/protobuf/proto" ) @@ -1088,10 +1090,19 @@ func blockDiscardOfZoneConfigForMultiRegionObject( // The change is permitted iff it is not modifying a protected multi-region // field of the zone configs (as defined by zonepb.MultiRegionZoneConfigFields). func (p *planner) CheckZoneConfigChangePermittedForMultiRegion( - ctx context.Context, zs tree.ZoneSpecifier, options tree.KVOptions, force bool, + ctx context.Context, zs tree.ZoneSpecifier, options tree.KVOptions, ) error { - // If the user has specified the FORCE option, the world is their oyster. - if force { + // If the user has specified that they're overriding, then the world is + // their oyster. + if p.SessionData().OverrideMultiRegionZoneConfigEnabled { + // Note that we increment the telemetry counter unconditionally here. + // It's possible that this will lead to over-counting as the user may + // have left the override on and is now updating a zone configuration + // that is not protected by the multi-region abstractions. To get finer + // grained counting however, would be more difficult to code, and may + // not even prove to be that valuable, so we have decided to live with + // the potential for over-counting. + telemetry.Inc(sqltelemetry.OverrideMultiRegionZoneConfigurationUser) return nil } @@ -1130,8 +1141,9 @@ func (p *planner) CheckZoneConfigChangePermittedForMultiRegion( hint := "to override this error, SET override_multi_region_zone_config = true and reissue the command" - // The request is to discard the zone configuration. Error in all discard - // cases. + // The request is to discard the zone configuration. Error in cases where + // the zone configuration being discarded was created by the multi-region + // abstractions. if options == nil { needToError := false // Determine if this zone config that we're trying to discard is @@ -1411,6 +1423,7 @@ func (p *planner) validateZoneConfigForMultiRegionDatabaseWasNotModifiedByUser( ) error { // If the user is overriding, our work here is done. if p.SessionData().OverrideMultiRegionZoneConfigEnabled { + telemetry.Inc(sqltelemetry.OverrideMultiRegionDatabaseZoneConfigurationSystem) return nil } currentZoneConfig, err := getZoneConfigRaw(ctx, p.txn, p.ExecCfg().Codec, dbDesc.GetID()) @@ -1484,6 +1497,7 @@ func (p *planner) validateZoneConfigForMultiRegionTableWasNotModifiedByUser( // If the user is overriding, or this is not a multi-region table our work here // is done. if p.SessionData().OverrideMultiRegionZoneConfigEnabled || desc.GetLocalityConfig() == nil { + telemetry.Inc(sqltelemetry.OverrideMultiRegionTableZoneConfigurationSystem) return nil } currentZoneConfig, err := getZoneConfigRaw(ctx, p.txn, p.ExecCfg().Codec, desc.GetID()) diff --git a/pkg/sql/set_zone_config.go b/pkg/sql/set_zone_config.go index 2a37fcecb92d..679e59dbf4b1 100644 --- a/pkg/sql/set_zone_config.go +++ b/pkg/sql/set_zone_config.go @@ -194,7 +194,6 @@ func (p *planner) SetZoneConfig(ctx context.Context, n *tree.SetZoneConfig) (pla ctx, n.ZoneSpecifier, n.Options, - p.SessionData().OverrideMultiRegionZoneConfigEnabled, ); err != nil { return nil, err } diff --git a/pkg/sql/sqltelemetry/multiregion.go b/pkg/sql/sqltelemetry/multiregion.go index f91f6f318def..02a287faabe3 100644 --- a/pkg/sql/sqltelemetry/multiregion.go +++ b/pkg/sql/sqltelemetry/multiregion.go @@ -56,6 +56,26 @@ var ( ImportIntoMultiRegionDatabaseCounter = telemetry.GetCounterOnce( "sql.multiregion.import", ) + + // OverrideMultiRegionZoneConfigurationUser is to be incremented when a + // multi-region zone configuration is overridden by the user. + OverrideMultiRegionZoneConfigurationUser = telemetry.GetCounterOnce( + "sql.multiregion.zone_configuration.override.user", + ) + + // OverrideMultiRegionDatabaseZoneConfigurationSystem is to be incremented + // when a multi-region database zone configuration is overridden by the + // system. + OverrideMultiRegionDatabaseZoneConfigurationSystem = telemetry.GetCounterOnce( + "sql.multiregion.zone_configuration.override.system.database", + ) + + // OverrideMultiRegionTableZoneConfigurationSystem is to be incremented when + // a multi-region table/index/partition zone configuration is overridden by + // the system. + OverrideMultiRegionTableZoneConfigurationSystem = telemetry.GetCounterOnce( + "sql.multiregion.zone_configuration.override.system.table", + ) ) // CreateDatabaseSurvivalGoalCounter is to be incremented when the survival goal