[prometheusremotewriteexporter] Allow to disable sanitize metric labels #8270

newly12 · 2022-03-07T06:11:30Z

Description:

This PR does the similar thing as #7112 , while the previous PR covers external labels defined in config, this PR follow the same config and allows to disable sanitize labels from scrapped metrics.

Link to tracking Issue:

Testing:

Documentation:

…ic labels

This reverts commit 8aee10b.

jpkrohling · 2022-03-07T18:34:54Z

Is this a bug? A feature request? Is there an issue tracking this?

Aneurysm9 · 2022-03-07T19:41:03Z

exporter/prometheusremotewriteexporter/exporter.go

@@ -107,7 +109,7 @@ func (prwe *prwExporter) PushMetrics(ctx context.Context, md pdata.Metrics) erro
 	case <-prwe.closeChan:
 		return errors.New("shutdown has been called")
 	default:
-		tsMap, err := prometheusremotewrite.FromMetrics(md, prometheusremotewrite.Settings{Namespace: prwe.namespace, ExternalLabels: prwe.externalLabels})
+		tsMap, err := prometheusremotewrite.FromMetrics(md, prometheusremotewrite.Settings{Namespace: prwe.namespace, ExternalLabels: prwe.externalLabels, SanitizeLabel: prwe.sanitizeLabel})


Can this be done without changing the exported interface of the PRW translator package?

I don't see much options, one is to introduce a feature flag in the translator package, but I was thinking to reuse the same feature flag as basically it's about the same thing as #7112 , thus an additional fields in prometheusremotewrite.Settings will be introduced to follow previous behavior by default.

The thing is that feature gates are intended to be temporary and removed eventually. Changes to the public API of a package are not reversible (assuming a world where we're v1.x, which we need to start planning for) and thus should be avoided in conjunction with feature gates. It is hypothesized that the sanitization of labels starting with _ (but not __) is unnecessary, so perhaps we should focus on finding a way to gain confidence in that hypothesis and eliminating the need for this feature gate.

I understand the concern of changing the public API in translator, but it's still the same behavior by default, with regards to gain confidence in the hypothesis, I am not sure how we can move forward on that. could you please elaborate?

hi @Aneurysm9 any thoughts on how we can move forward on this?

Then we should probably eliminate it altogether rather than further propagating it. Changing the public API of this module does not seem like the correct thing to do.

I'm not sure I'm comfortable making this decision by myself. @dashpole @bogdandrutu @jpkrohling @codeboten @gouthamve

yeah that sounds like this will be a breaking change which is going to remove the key prefix.

If not to change the public API, would it work that introduce a new method similar to FromMetrics which does not sanitize label(no key prefix)?

Changing the public API of this module does not seem like the correct thing to do.

I share the same opinion.

I also agree we should not change the public API. We should either make the change outright, or use a feature-gate to slowly make the change.

Added a feature gate for this.

newly12 · 2022-03-08T00:29:11Z

@jpkrohling it's a feature request. I searched issues and didn't see one for this.

Signed-off-by: Peter Deng <[email protected]> # Conflicts: # CHANGELOG.md # pkg/translator/prometheusremotewrite/helper.go # pkg/translator/prometheusremotewrite/helper_test.go # pkg/translator/prometheusremotewrite/testutils_test.go

Signed-off-by: Peter Deng <[email protected]> # Conflicts: # CHANGELOG.md # pkg/translator/prometheusremotewrite/helper.go

… conflicts

CHANGELOG.md

# Conflicts: # CHANGELOG.md

changelog fixed

jpkrohling · 2022-03-22T12:53:43Z

Once @Aneurysm9 approves, I'll merge this.

# Conflicts: # CHANGELOG.md # pkg/translator/prometheusremotewrite/helper.go

dashpole

It looks like this misses some calls to sanitize elsewhere in helper.go. It seems like it would be simpler to just check the feature gate from within sanitize():

-	if s[0] == '_' {
+	if !featuregate.IsEnabled(dropSanitizationGate.ID) && s[0] == '_' {
		s = keyStr + s
	}

CHANGELOG.md

newly12 · 2022-04-12T02:35:24Z

It looks like this misses some calls to sanitize elsewhere in helper.go. It seems like it would be simpler to just check the feature gate from within sanitize():
-	if s[0] == '_' {
+	if !featuregate.IsEnabled(dropSanitizationGate.ID) && s[0] == '_' {
		s = keyStr + s
	}

good point. updated.

newly12 · 2022-04-15T02:22:07Z

@dashpole @Aneurysm9 @jpkrohling could you please review again?

CHANGELOG.md

Co-authored-by: David Ashpole <[email protected]>

[pkg/translator/prometheusremotewrite] Allow to disable sanitize metr…

93b59d4

…ic labels

newly12 requested a review from a team March 7, 2022 06:11

newly12 requested a review from Aneurysm9 as a code owner March 7, 2022 06:11

github-actions bot assigned Aneurysm9 Mar 7, 2022

update CHANGELOG

ee2d5f3

newly12 changed the title ~~[pkg/translator/prometheusremotewrite] Allow to disable sanitize metric labels~~ [exporter/prometheusremotewrite] Allow to disable sanitize metric labels Mar 7, 2022

newly12 changed the title ~~[exporter/prometheusremotewrite] Allow to disable sanitize metric labels~~ [prometheusremotewriteexporter] Allow to disable sanitize metric labels Mar 7, 2022

newly12 added 3 commits March 7, 2022 14:30

update CHANGELOG

57c4158

export sanitizeLabel in config

8aee10b

Revert "export sanitizeLabel in config"

053deb9

This reverts commit 8aee10b.

Aneurysm9 reviewed Mar 7, 2022

View reviewed changes

newly12 added 4 commits March 8, 2022 08:29

Merge branch 'main' into label_sanitize

ea10855

Merge remote-tracking branch 'origin/main' into label_sanitize

c135782

Signed-off-by: Peter Deng <[email protected]> # Conflicts: # CHANGELOG.md # pkg/translator/prometheusremotewrite/helper.go

fix deprecated pdata.AttributeValue and issue introduced while fixing…

1902878

… conflicts

jpkrohling previously requested changes Mar 21, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into label_sanitize

5725154

# Conflicts: # CHANGELOG.md

newly12 added 3 commits April 9, 2022 14:16

Merge remote-tracking branch 'origin/main' into label_sanitize

a1470f8

# Conflicts: # CHANGELOG.md # pkg/translator/prometheusremotewrite/helper.go

move sanitizeLabel switch to feature gate

0203fc5

update CHANGELOG

c75b35d

dashpole requested changes Apr 11, 2022

View reviewed changes

jpkrohling reviewed Apr 11, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

move feature gate check in sanitize method

1fbb965

newly12 added 2 commits April 12, 2022 10:36

Merge branch 'main' into label_sanitize

d6c9c3d

fix CHANGELOG

3cf41e1

dashpole approved these changes Apr 12, 2022

View reviewed changes

newly12 added 3 commits April 14, 2022 09:15

Merge branch 'main' into label_sanitize

99123a2

fmt and fix UT

02e5a9f

Merge branch 'main' into label_sanitize

8469afb

dashpole reviewed Apr 15, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

3848f57

Co-authored-by: David Ashpole <[email protected]>

jpkrohling merged commit cae2803 into open-telemetry:main Apr 18, 2022

newly12 deleted the label_sanitize branch April 21, 2022 01:57

ahayworth mentioned this pull request May 16, 2022

Update to otel-contrib-col v0.51.0 Shopify/opentelemetry-collector-contrib#796

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[prometheusremotewriteexporter] Allow to disable sanitize metric labels #8270

[prometheusremotewriteexporter] Allow to disable sanitize metric labels #8270

newly12 commented Mar 7, 2022

jpkrohling commented Mar 7, 2022 •

edited

Loading

Aneurysm9 Mar 7, 2022

newly12 Mar 8, 2022

Aneurysm9 Mar 9, 2022

newly12 Mar 10, 2022

newly12 Mar 17, 2022

Aneurysm9 Mar 30, 2022

newly12 Mar 31, 2022

jpkrohling Mar 31, 2022

dashpole Mar 31, 2022

newly12 Apr 9, 2022

newly12 commented Mar 8, 2022

jpkrohling commented Mar 22, 2022

dashpole left a comment

newly12 commented Apr 12, 2022

newly12 commented Apr 15, 2022

[prometheusremotewriteexporter] Allow to disable sanitize metric labels #8270

[prometheusremotewriteexporter] Allow to disable sanitize metric labels #8270

Conversation

newly12 commented Mar 7, 2022

jpkrohling commented Mar 7, 2022 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newly12 commented Mar 8, 2022

jpkrohling commented Mar 22, 2022

dashpole left a comment

Choose a reason for hiding this comment

newly12 commented Apr 12, 2022

newly12 commented Apr 15, 2022

jpkrohling commented Mar 7, 2022 •

edited

Loading