Make aggregationFile an optional setting

[fionaliao]

aggregationFile is currently a mandatory setting for configuring a GrafanaNet route. This PR updates it to be optional. Making this optional simplifies the process of migrating to Grafana Cloud Graphite V5.

If aggregationFile is not set, the most recent aggregation file uploaded to Grafana Cloud will be used instead. If no aggregation files have been uploaded, the default Grafana Cloud aggregations will be applied.

This is a breaking change for the addRoute grafanaNet ... command, as aggregationFile= needs to be prepended to the aggregation file value since it's now an optional parameter.

Testing

Built a local Docker image for my branch and configured it to push to Grafana Cloud. Checked that ignoring aggregationFile doesn't error, and also checked that setting aggregationFile causes the config to be pushed to Grafana Cloud.

[jesusvazquez]

Why do you need to do this here?

[fionaliao]

The value of t.Token when we enter the for loop will be the name of the optional setting (in this case aggregationFile=). We need to move to the next token to get the value of the setting.

[jesusvazquez]

Overall looks good to me 💪

[Dieterbe]

There's still a couple places that need updating.

e.g. docs/grafana-net.md (make it clear it's optional now), jsonnet (remove the default)
i recommend you grep for aggregations throughout the repo to find what needs updating.

[fionaliao]

i recommend you grep for aggregations throughout the repo to find what needs updating.

This has been done, mainly in these two commits: https://github.com/grafana/carbon-relay-ng/pull/485/files/0379100cd31a361bbf0fcb3b7da04e9f7219d88a..414da429ba9bd73f696938cf7c843d8b0faa0db3

jsonnet (remove the default)

I decided to keep the aggregation file config in the jsonnet, but only add the file to the config map if $._config.storage_aggregation was not null:

carbon-relay-ng/jsonnet/lib/carbon-relay-ng/crng.libsonnet

Lines 14 to 32 in 414da42

	storage_schemas: importstr 'files/storage-schemas.conf',
	// You can set storage_aggregation to null if you don't have an
	// aggregation file and want to use Grafana Cloud's default aggregations
	// instead. (And in that case, don't forget to remove the aggregationFile
	// setting in carbon-relay-ng.ini.)
	storage_aggregation: importstr 'files/storage-aggregation.conf',
	},
	local configMap = k.core.v1.configMap,
	carbon_relay_ng_config_map:
	configMap.new('carbon-relay-ng-config') +
	configMap.mixin.metadata.withNamespace($._config.namespace) +
	configMap.withData(
	{
	'carbon-relay-ng.ini': $._config.crng_config,
	'storage-schemas.conf': $._config.storage_schemas,
	[if $._config.storage_aggregation != null then 'storage-aggregation.conf']: $._config.storage_aggregation,
	}
	),

The reasoning was that having the aggregation file and configmap set up is a useful reference if you want to configure your aggregations, but there are also simple and clear steps you can take if you don't want to send an aggregation file.

[Dieterbe]

I decided to keep the aggregation file config in the jsonnet

ok. does the default match what we have in cloud v5?

[Dieterbe]

why? and why do we do it differently here than in NewGrafanaNetConfig() ?

[fionaliao]

This just wraps the original (pre-PR) ReadAggregations(...) call in an if-else because it's possible that no file will be supplied.

I see you're commenting against a diff within two commits of the PR though:

In an earlier commit, I added the "NewGrafanaNet:..." error since I'd removed the aggregation file parameter from NewGrafanaNetConfig(). After the suggestion to keep the aggregation file parameter, I reverted that change and then reverted the error in this method back to what it was originally - just returning the error from reading the aggregations.

[fionaliao]

ok. does the default match what we have in cloud v5?

It didn't, but I updated the examples to match in e4294f0

[Dieterbe]

nice work, fiona. thanks!