[VAULT-2776] Add prefix_filter option to Vault #12025

pmmukh · 2021-07-08T21:32:24Z

Adds a prefix_filter option to the telemetry configs, that can explicitly allow or block certain metric prefixes.
Adds a filter_default option to the telemetry configs, which is required in conjunction with the config above, to decide whether all metrics are allowed or denied by default. This is currently always true, but users may want to control this in their vault config along with prefix_filter.
Updates documentation to cover both configs

Note that this attempts to maintain parity with this config in consul https://www.consul.io/docs/agent/options#telemetry-prefix_filter
Also, the logic for parsing the prefixFilter config is pretty much lifted from here https://github.com/hashicorp/consul/blob/main/agent/config/builder.go#L640-L654

Related GH issue: #3792

ncabatoff · 2021-07-08T22:47:34Z

internalshared/configutil/telemetry.go

+		case '-':
+			telemetryBlockedPrefixes = append(telemetryBlockedPrefixes, rule[1:])
+		default:
+			log.Printf("Filter rule must begin with either '+' or '-': %q", rule)


We never use the stdlib log in Vault. I would add an error return value to this func instead.

Ah yeah I wasn't sure if I should use log here. I think the only reason I didn't do error return, is cause the equivalent consul code logs out a warning when the filter is improperly formatted versus failing https://github.com/hashicorp/consul/blob/main/agent/config/builder.go#L652, so I was looking to maintain that pattern. Maybe opts.UI.Warn would make sense https://github.com/hashicorp/vault/blob/main/internalshared/configutil/telemetry.go#L371 ?

ncabatoff · 2021-07-08T22:49:23Z

website/content/docs/configuration/telemetry.mdx

@@ -51,6 +51,17 @@ The following options are available on all telemetry configurations.
 - `add_lease_metrics_namespace_labels` `(bool: false)` - If this value is set to true, then `vault.expire.leases.by_expiration`
  will break down expiring leases by both time and namespace. This parameter is disabled by default because enabling it can lead
  to a large-cardinality metric.
+- `filter_default` - This controls whether to allow metrics that have not been specified by the filter.


Please add (bool: true) to describe this param.

ncabatoff · 2021-07-08T22:51:42Z

internalshared/configutil/telemetry_test.go

+
+	allowedPrefixes, blockedPrefixes := parsePrefixFilter(prefixFilters)
+
+	assert.Equal(t, len(allowedPrefixes), 1)


You can say assert.Len to check the length with testify.

I ended up just doing an assert.Equal against an expected array, while refactoring the test
6a28af2

ncabatoff · 2021-07-08T22:59:19Z

I know Consul doesn't treat a config error here as a failure and will start up with warnings, but I would prefer we were stricter. I think it's a bad UX to ignore config errors.

Also, could you add fixture-based test(s) like we do for most other config parsing code? Look under command/server for some examples.

pmmukh · 2021-07-08T23:03:36Z

I know Consul doesn't treat a config error here as a failure and will start up with warnings, but I would prefer we were stricter. I think it's a bad UX to ignore config errors.

Roger, i would concur with that tbh, will change it to error out.

Also, could you add fixture-based test(s) like we do for most other config parsing code? Look under command/server for some examples.

Will do!

pmmukh · 2021-07-09T18:35:46Z

Also, could you add fixture-based test(s) like we do for most other config parsing code? Look under command/server for some examples.

6a28af2

ncabatoff · 2021-07-09T18:40:50Z

internalshared/configutil/telemetry_test.go

+
+			allowedPrefixes, blockedPrefixes, err := parsePrefixFilter(tc.inputFilters)
+
+			if err != nil {


It's fine with the cases you have now, but I consider it best practice when you have an expected error field in your test case type to do something like

if err != nil { compare with expected error }
else { assert expected error empty}

ah that makes sense! added that here 6c2ad99

command/server/config_telemetry_test.go

ncabatoff · 2021-07-09T18:51:50Z

.gitignore

@@ -48,7 +48,7 @@ Vagrantfile
 # Configs
 *.hcl
 !command/agent/config/test-fixtures/*.hcl
-!command/server/test-fixtures/*.hcl
+!command/server/test-fixtures/*/*.hcl


Should this be **/*.hcl instead of */*.hcl?

it should! beae728

ncabatoff

Looks good, possibly modulo the .gitignore change.

* [VAULT-2776] Add prefix_filter support to vault * [VAULT-2776] Add filter_default config, update docs * [VAULT-2776] Add changelog file * [VAULT-2776] Update telemetry tests and error handling * [VAULT-2776] Add test fixtures, update test * [VAULT-2776] Update gitignore hcl filter

pmmukh added 3 commits July 8, 2021 15:29

[VAULT-2776] Add prefix_filter support to vault

7305528

[VAULT-2776] Add filter_default config, update docs

6447939

[VAULT-2776] Add changelog file

73d3e97

vercel bot temporarily deployed to Preview – vault-storybook July 8, 2021 21:38 Inactive

vercel bot temporarily deployed to Preview – vault July 8, 2021 21:38 Inactive

pmmukh requested review from vishalnayak, ncabatoff and swayne275 July 8, 2021 21:45

ncabatoff reviewed Jul 8, 2021

View reviewed changes

[VAULT-2776] Update telemetry tests and error handling

6a28af2

vercel bot temporarily deployed to Preview – vault-storybook July 9, 2021 18:34 Inactive

vercel bot deployed to Preview – vault July 9, 2021 18:34 View deployment

pmmukh requested a review from ncabatoff July 9, 2021 18:35

ncabatoff reviewed Jul 9, 2021

View reviewed changes

command/server/config_telemetry_test.go Show resolved Hide resolved

[VAULT-2776] Add test fixtures, update test

6c2ad99

vercel bot temporarily deployed to Preview – vault July 9, 2021 18:48 Inactive

vercel bot temporarily deployed to Preview – vault-storybook July 9, 2021 18:48 Inactive

pmmukh requested a review from ncabatoff July 9, 2021 18:49

ncabatoff reviewed Jul 9, 2021

View reviewed changes

ncabatoff approved these changes Jul 9, 2021

View reviewed changes

[VAULT-2776] Update gitignore hcl filter

beae728

vercel bot temporarily deployed to Preview – vault-storybook July 9, 2021 18:57 Inactive

vercel bot temporarily deployed to Preview – vault July 9, 2021 18:57 Inactive

pmmukh merged commit c7b8291 into main Jul 9, 2021

pmmukh deleted the add-telemetry-prefix-option branch July 9, 2021 19:49

pmmukh mentioned this pull request Jul 9, 2021

[improvement] Add prefix_filter option for telemetry #3792

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VAULT-2776] Add prefix_filter option to Vault #12025

[VAULT-2776] Add prefix_filter option to Vault #12025

pmmukh commented Jul 8, 2021 •

edited

ncabatoff Jul 8, 2021

pmmukh Jul 8, 2021

pmmukh Jul 9, 2021

ncabatoff Jul 8, 2021 •

edited

pmmukh Jul 9, 2021

ncabatoff Jul 8, 2021

pmmukh Jul 9, 2021

ncabatoff commented Jul 8, 2021

pmmukh commented Jul 8, 2021 •

edited

pmmukh commented Jul 9, 2021

ncabatoff Jul 9, 2021

pmmukh Jul 9, 2021

ncabatoff Jul 9, 2021

pmmukh Jul 9, 2021

ncabatoff left a comment


		allowedPrefixes, blockedPrefixes := parsePrefixFilter(prefixFilters)

		assert.Equal(t, len(allowedPrefixes), 1)


		allowedPrefixes, blockedPrefixes, err := parsePrefixFilter(tc.inputFilters)

		if err != nil {

[VAULT-2776] Add prefix_filter option to Vault #12025

[VAULT-2776] Add prefix_filter option to Vault #12025

Conversation

pmmukh commented Jul 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncabatoff Jul 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncabatoff commented Jul 8, 2021

pmmukh commented Jul 8, 2021 • edited

pmmukh commented Jul 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncabatoff left a comment

Choose a reason for hiding this comment

pmmukh commented Jul 8, 2021 •

edited

ncabatoff Jul 8, 2021 •

edited

pmmukh commented Jul 8, 2021 •

edited