ccl, sql: add telemetry, logs for feature denials by feature flags#57328
Conversation
|
This change is |
angelapwen
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @joshimhoff, @miretskiy, and @otan)
pkg/sql/sqltelemetry/feature_flags.go, line 17 at r1 (raw file):
// FeatureDeniedByFeatureFlagCounter is a counter that is incremented every time a feature is // denied via the feature flag cluster setting, for example. feature.schema_change.enabled = FALSE. var FeatureDeniedByFeatureFlagCounter = telemetry.GetCounterOnce("sql.feature_flag.denied")
I wasn't quite sure whether this should be considered within the sql category of telemetry considering it should log from other packages. But given that it is under sqltelemetry folder and that other ccl tests also use sql.* for telemetry I left it this way.
pkg/sql/testdata/telemetry/feature_flags, line 24 at r1 (raw file):
sql.feature_flag.denied # Test that an IMPORT is counted.
I cannot run the Bulk IO commands in this package, but also do not see where telemetry can be tested within the ccl package. I see some logic tests in ccl/logictestccl but the relevant commands also cannot be accessed in that directory. Do you think it is necessary to test the telemetry for Bulk IO features considering I already have the schema change and CREATE STATISTICS/ANALYZE tested? It certainly should work as the counter is incremented within the featureflag.CheckEnabled method.
713b65a to
bc79c94
Compare
otan
left a comment
There was a problem hiding this comment.
Reviewed 1 of 44 files at r1.
Reviewable status:
pkg/featureflag/feature_flags.go, line 39 at r2 (raw file):
log.Warningf( ctx,
this logging may get excessive. maybe let's try with a higher verbosity, i.e. if log.V(2) { log.Warningf(ctx, ...) }
pkg/sql/sqltelemetry/feature_flags.go, line 17 at r1 (raw file):
Previously, angelapwen (Angela P Wen) wrote…
I wasn't quite sure whether this should be considered within the
sqlcategory of telemetry considering it should log from other packages. But given that it is undersqltelemetryfolder and that otherccltests also usesql.*for telemetry I left it this way.
hmm, yeah this is fine!
pkg/sql/testdata/telemetry/feature_flags, line 24 at r1 (raw file):
Previously, angelapwen (Angela P Wen) wrote…
I cannot run the Bulk IO commands in this package, but also do not see where telemetry can be tested within the
cclpackage. I see some logic tests inccl/logictestcclbut the relevant commands also cannot be accessed in that directory. Do you think it is necessary to test the telemetry for Bulk IO features considering I already have the schema change and CREATE STATISTICS/ANALYZE tested? It certainly should work as the counter is incremented within thefeatureflag.CheckEnabledmethod.
it's okay, i think you can skip it :)
angelapwen
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/featureflag/feature_flags.go, line 39 at r2 (raw file):
Previously, otan (Oliver Tan) wrote…
this logging may get excessive. maybe let's try with a higher verbosity, i.e.
if log.V(2) { log.Warningf(ctx, ...) }
Thanks that makes sense!!
pkg/sql/sqltelemetry/feature_flags.go, line 17 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
hmm, yeah this is fine!
Cool thanks!
pkg/sql/testdata/telemetry/feature_flags, line 24 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
it's okay, i think you can skip it :)
Yay okay!
bc79c94 to
a253e7c
Compare
otan
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/featureflag/feature_flags.go, line 39 at r2 (raw file):
Previously, angelapwen (Angela P Wen) wrote…
Thanks that makes sense!!
you can do this in a later PR, but were you planning to add a graph to the admin UI as well?
|
pkg/featureflag/feature_flags.go, line 39 at r2 (raw file): Previously, otan (Oliver Tan) wrote…
Ah... you're right, I had thought the time series graph was somehow created automatically but I realize that's not true 😸 |
pkg/sql/set_zone_config.go
Outdated
| // This can be either a literal NULL (deletion), or a string containing YAML. | ||
| // We also support byte arrays for backward compatibility with | ||
| // previous versions of CockroachDB. | ||
| // previous versions of CockrofachDB. |
There was a problem hiding this comment.
Minor nit s/CockrofachDB/CockroachDB/g
1af557d to
644311a
Compare
This change adds a telemetry counter, sql.feature_flag_denied, in the case where a feature is denied by its corresponding feature flag being set to disabled in cluster settings. It also adds a level 2 log warning for the database administrator to view the denied command as well as its optional category. Release note: none
644311a to
2bd3d11
Compare
|
bors r=otan |
|
Build succeeded: |
This change adds a telemetry counter, sql.feature_flag.denied, in
the case where a feature is denied by its corresponding feature flag
being set to disabled in cluster settings. It also adds a log
warning for the database administrator to view the denied command
as well as its optional category.
Release note: none