Make intrinsic dimensions configurable and disable status_message by default by stoewer · Pull Request #1960 · grafana/tempo

stoewer · 2022-12-16T06:36:02Z

What this PR does:

The metrics generator always adds the intrinsic dimensions service, span_name, span_kind, status_code, and status_message. The status_message may contain arbitrary strings and can have a very high cardinality (see #1932). Therefore it is safer not to add this dimension by default.

With this PR:

All above mentioned intrinsic dimensions are configurable via configuration file and overrides.
The status_message is not added by default.

Which issue(s) this PR fixes:
Fixes #1932

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

mapno

LGTM. If you can re-generate docs/tempo/website/configuration/manifest.md, that'd be great. It's due an update.

joe-elliott

Some thoughts.

joe-elliott · 2022-12-16T13:52:22Z

    [metrics_generator_processor_service_graphs_histogram_buckets: <list of float>]
    [metrics_generator_processor_service_graphs_dimensions: <list of string>]
    [metrics_generator_processor_span_metrics_histogram_buckets: <list of float>]
+    [metrics_generator_processor_span_metrics_intrinsic_dimensions: <map string to bool>]


may want to add a quick note about what strings are legal as keys

joe-elliott · 2022-12-16T13:58:50Z

 	return m.spanMetricsDimensions
 }
+
+func (m *mockOverrides) MetricsGeneratorProcessorSpanMetricsIntrinsicDimensions(userID string) map[string]bool {


extend TestProcessorConfig_copyWithOverrides to include the intrinsic dimensions?

Also might be worth adding a case to Test_instance_updateProcessors to make sure the diff works with the intrinsics.

Sound good. I added both tests.

joe-elliott · 2022-12-16T14:05:06Z

+	if cfg.IntrinsicDimensions.StatusMessage {
+		labels = append(labels, dimStatusMessage)
+	}
+	intrinsicDimensions := labels[:]


why copy the slice here? it's pointing to the same underlying array.

This was needed in order to detect collisions with configured intrinsic dimensions. This is no longer needed as dimensions are now also treated as collisions when the colliding intrinsic dimension is absent.

joe-elliott · 2022-12-16T14:09:13Z

 }

-func sanitizeLabelNameWithCollisions(name string) string {
+func sanitizeLabelNameWithCollisions(name string, intrinsicDimensions []string) string {


sanitizing attribute names only if they conflict with configured intrinsics has some weird properties. if someone configures status_message as a normal attribute and then later adds the matching intrinsic then the name of the normal attribute label changes. this may break dashboards or queries b/c it changes the meaning of their metric labels. similar situations can occur when disabling intrinsic dimensions.

my instinct is to sanitize against all intrinsics even if they are not configured.

joe-elliott · 2022-12-16T14:12:30Z

 * [BUGFIX] New wal file separator '+' for the NTFS filesystem and backward compatibility with the old separator ':' [#1700](https://github.com/grafana/tempo/pull/1700) (@kilian-kier)
 * [ENHANCEMENT] metrics-generator: extract `status_message` field from spans [#1786](https://github.com/grafana/tempo/pull/1786), [#1794](https://github.com/grafana/tempo/pull/1794) (@stoewer)
 * [ENHANCEMENT] metrics-generator: handle collisions between user defined and default dimensions [#1794](https://github.com/grafana/tempo/pull/1794) (@stoewer)
+* [ENHANCEMENT] metrics-generator: make intrinsic dimensions configurable and disable `status_message` by default [#1960](https://github.com/grafana/tempo/pull/1960) (@stoewer)


i believe there's a breaking change here (or perhaps one in #1786) that should be mentioned. If the user has a configured normal attribute dimension called status_message then it's name will change to __status_message.

See below for comments on the renaming behavior.

Agreed. I think the actual breaking change was in #1794. I marked the change as breaking in the changelog.

joe-elliott · 2022-12-16T14:15:06Z

+	MetricsGeneratorProcessorServiceGraphsDimensions        []string        `yaml:"metrics_generator_processor_service_graphs_dimensions" json:"metrics_generator_processor_service_graphs_dimensions"`
+	MetricsGeneratorProcessorSpanMetricsHistogramBuckets    []float64       `yaml:"metrics_generator_processor_span_metrics_histogram_buckets" json:"metrics_generator_processor_span_metrics_histogram_buckets"`
+	MetricsGeneratorProcessorSpanMetricsDimensions          []string        `yaml:"metrics_generator_processor_span_metrics_dimensions" json:"metrics_generator_processor_span_metrics_dimensions"`
+	MetricsGeneratorProcessorSpanMetricsIntrinsicDimensions map[string]bool `yaml:"metrics_generator_processor_span_metrics_intrinsic_dimensions" json:"metrics_generator_processor_span_metrics_intrinsic_dimensions"`


should we fail to load overrides if a value is specified here that is not an intrinsic dimension?

metrics_generator_processor_span_metrics_intrinsic_dimensions: status_message: true foo: false

I could not think of a good way to validate they intrinsic dimensions while loading the overrides without running into import cycles or repeating dimension definitions in the overrides package.

I solved it now by checking for valid keys when the overrides are applied.

stoewer · 2022-12-19T08:27:40Z

If you can re-generate docs/tempo/website/configuration/manifest.md, that'd be great. It's due an update.

Good idea. I regenerated it.

The linters deadcode, structcheck, and varcheck are deprecated since golangci-lint v1.49.0. They are replace by unused which is already enabled in our config

Custom dimensions from span attributs are also treated as conflicts when the conflicting intrinsic dimension is not actually present. This prevents those dimensions from being renamed when the intrinsic dimension configuration changes

mapno

LGTM, nice work

stoewer changed the title ~~Optional status message~~ Make intrinsic dimensions configurable and disable status_message by default Dec 16, 2022

mapno reviewed Dec 16, 2022

View reviewed changes

joe-elliott reviewed Dec 16, 2022

View reviewed changes

stoewer force-pushed the optional-status-message branch from 6db8e29 to eec42ea Compare December 19, 2022 07:15

stoewer added 10 commits December 20, 2022 08:55

Add intrinsic dimenstions to spanmetrics config

f21da45

Remove deprecated linters from glangci-lint config

67bd81e

The linters deadcode, structcheck, and varcheck are deprecated since golangci-lint v1.49.0. They are replace by unused which is already enabled in our config

Make intrinsic dimensions configurable for spanmetrics

b0ebdf0

Disable intrinsic dimension status_message by default

ee9ca98

Add overrides for intrinsic dimension configuration

76f0fed

Docs for the configuration of intrinsic dimensions

ead9f43

Add PR to changelog

e5a69fd

Regenerate configuration schema in manifest.md

49411b2

Mark change grafana#1794 as breaking in changelog

e219a6c

stoewer force-pushed the optional-status-message branch from ba5aedd to 6ac650a Compare December 19, 2022 23:49

stoewer added 2 commits December 20, 2022 11:43

Unit test for intrinsic dimension override

45b6e36

Fail on invalid intrinsic dimension override keys

9088882

stoewer force-pushed the optional-status-message branch from 47e95d3 to 9088882 Compare December 20, 2022 01:44

Fix dimension prefix in changelog

027a59a

stoewer force-pushed the optional-status-message branch from 416ffd8 to 027a59a Compare December 20, 2022 01:55

stoewer marked this pull request as ready for review December 20, 2022 02:10

stoewer requested review from annanay25, knylander-grafana, mdisibio, yvrhdn and zalegrala as code owners December 20, 2022 02:10

mapno approved these changes Dec 20, 2022

View reviewed changes

mapno merged commit 0ed9ddc into grafana:main Dec 21, 2022

stoewer deleted the optional-status-message branch March 20, 2023 07:04

Conversation

stoewer commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mapno left a comment

Choose a reason for hiding this comment

Uh oh!

joe-elliott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoewer Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoewer commented Dec 19, 2022

Uh oh!

mapno left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stoewer commented Dec 16, 2022 •

edited

Loading

stoewer Dec 20, 2022 •

edited

Loading