Allow empty separator in relabel config #6425

haanhvu · 2024-03-23T11:31:36Z

Description

Currently we don't allow empty separator in relabel config. If users specify an empty separator, it will be turned into the default ; from Prometheus side. Since Prometheus makes use of empty separator, Prometheus operator should do that too.

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

CHANGE (fix or feature that would cause existing functionality to not work as expected)
FEATURE (non-breaking change which adds functionality)
BUGFIX (non-breaking change which fixes an issue)
ENHANCEMENT (non-breaking change which improves existing functionality)
NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Verification

No tests broken. New tests passed.

Changelog entry

Allow empty seperator in relabel config

Question

Is this considered a breaking change?

haanhvu · 2024-03-23T16:56:08Z

@slashpai Could you review?

Basically, I followed your PR. Just did 2 changes:

Change //+kubebuilder:default=; to //+kubebuilder:default=";". Without "" it would generate array type instead of string type, as you could see in your PR.
Fixed tests

slashpai · 2024-03-25T06:45:24Z

Thanks for taking this up.
Did you test with a local cluster if changes are working?

haanhvu · 2024-03-25T08:56:35Z

Thanks for taking this up. Did you test with a local cluster if changes are working?

I ran with ./scripts/run-external.sh -c and it ran successfully. But as you can see, some e2e are failing. At first I thought it was flaky but now looked deeper into the code I see the failure may relate to my change.

I think I need to look deeper into how this is handled in Prometheus codebase. From my experience label-related changes are very bug-prone. Don't want to reinvent the wheel and create a bunch of subtle bugs...

simonpasquier · 2024-03-25T16:04:20Z

pkg/apis/monitoring/v1/prometheus_types.go

@@ -1497,7 +1497,8 @@ type RelabelConfig struct {
 	SourceLabels []LabelName `json:"sourceLabels,omitempty"`

 	// Separator is the string between concatenated SourceLabels.
-	Separator string `json:"separator,omitempty"`
+	//+kubebuilder:default=";"
+	Separator string `json:"separator"`


The "issue" with adding a default value is that the API response will change to return separator: ";" if the field isn't set and it may disrupt some clients (reconcilers for instance).

Another option would be to change the field type to be a string pointer:

if the pointer is nil, don't set the separator field in the generated Prometheus config (and let Prometheus use its own default).

if not nil, pass the value as-is.

The risk with this option is for users that have separator: "" in their YAML manifests today as it will change the behavior for them.

Suggested change

Separator string `json:"separator"`

Separator *string `json:"separator"`

Yes I agree this is a breaking change. But since this change is on par with Prometheus's behavior, I think it's worth doing.

I thought of your suggested approach too. Will try this. I think it is less risky than the current (default ; approach) because there might be less users setting separator: "" than users not setting it at all.

Agreed, I don't think that separator: "" is widely used in the wild, especially knowing that as of today, it's the same as not setting it. If we follow that path, we's till need to mark it as a CHANGE in the changelog for safety.

haanhvu · 2024-03-25T19:41:17Z

@slashpai @simonpasquier All tests are passed now. Do you think we need to add tests for "" separator is correctly used (not ignored or replaced by ; from Prometheus side)? If yes, where should I add those tests?

simonpasquier · 2024-03-26T08:06:45Z

I would extend TestAdditionalAlertRelabelConfigs to have a relabel config rule setting separator: "".

mviswanathsai · 2024-03-31T09:37:37Z

pkg/prometheus/resource_selector.go

-			!(rc.Separator == "" ||
-				rc.Separator == relabel.DefaultRelabelConfig.Separator) ||
+			!(rc.Separator == nil ||
+				rc.Separator == &relabel.DefaultRelabelConfig.Separator) ||


The addresses will always be different. Yes, it will be the same if at some point rc.Separator = (default value in rhs) in some languages but not in go, see this. I think it might be better to dereference rc.Separator instead of getting the address of the default value.

Thanks. I know what you mean. I was just too focused on the nil separator, since that's the breaking change. Will fix this in next commit.

mviswanathsai · 2024-03-31T09:38:38Z

pkg/prometheus/resource_selector.go

-			!(rc.Separator == "" ||
-				rc.Separator == relabel.DefaultRelabelConfig.Separator) ||
+			!(rc.Separator == nil ||
+				rc.Separator == &relabel.DefaultRelabelConfig.Separator) ||


Same here. Check this playground out.

mviswanathsai · 2024-03-31T10:37:43Z

test/e2e/prometheus_test.go

@@ -4657,7 +4659,7 @@ func testRelabelConfigCRDValidation(t *testing.T) {
 			relabelConfigs: []*monitoringv1.RelabelConfig{
 				{
 					SourceLabels: []monitoringv1.LabelName{"__address__"},
-					Separator:    ",",
+					Separator:    &sep1,


(nit) you can use ptr.To instead.

Agree, using pointer package from k8s would be better.

I'll fix this nit tomorrow when I have more time.

mviswanathsai · 2024-03-31T10:37:56Z

test/e2e/prometheus_test.go

@@ -4668,7 +4670,7 @@ func testRelabelConfigCRDValidation(t *testing.T) {
 			scenario: "empty-separator",
 			relabelConfigs: []*monitoringv1.RelabelConfig{
 				{
-					Separator: "",
+					Separator: &sep2,


ArthurSens

Just some nitpicking about file names[1][2]

I totally understand that this is beyond the scope of this PR, I'll open a separate issue for that

ArthurSens · 2024-04-01T23:55:37Z

pkg/prometheus/testdata/AdditionalAlertRelabelConfigs.golden

+- sourceLabels: [label1, label2]
+  separator: ""
+  action: replace
+  targetLabel: custom_label


It took me forever to understand why such a simple change (changing separator to pointer), would change so much a golden file, e.g. source labels, action, etc.

That was until I realized that this file is not really a golden file by definition, but an input! We should change the suffix of this file to something else, e.g. .input

👍 btw the config format is wrong since it expects Prometheus snake-case format.

simonpasquier · 2024-04-02T13:38:26Z

pkg/prometheus/resource_selector_test.go

@@ -392,7 +392,7 @@ func TestValidateRelabelConfig(t *testing.T) {
 			relabelConfig: monitoringv1.RelabelConfig{
 				SourceLabels: []monitoringv1.LabelName{"__tmp_port"},
 				TargetLabel:  "__port1",
-				Separator:    relabel.DefaultRelabelConfig.Separator,
+				Separator:    &relabel.DefaultRelabelConfig.Separator,


(nit)

Suggested change

Separator: &relabel.DefaultRelabelConfig.Separator,

Separator: ptr.To(relabel.DefaultRelabelConfig.Separator),

mviswanathsai · 2024-04-02T19:14:31Z

You might want to squash these commits into one (or a few) once you are done with all the changes. You might already be on it, just a reminder!

haanhvu · 2024-04-02T19:20:15Z

You might want to squash these commits into one (or a few) once you are done with all the changes. You might already be on it, just a reminder!

I've never done this in any of my merged PRs here. Is there any reason to do that in this PR?

mviswanathsai · 2024-04-02T19:25:45Z

It's not really a necessity per se, but having one point in the final commit tree which tells you what exactly this commit did, than to have 10 or so commits that took to achieve it would be more desirable.
Ofcourse, in the PR itself there's nothing 'wrong" with having as many commits.

mviswanathsai · 2024-04-02T19:26:44Z

For example, "fix e2e failure" is meaningful within the context of this PR. But once merged to main, it would become rather ambiguous.
Again, it's just a nit. More of a good practice than a necessity or anything.

haanhvu · 2024-04-02T19:48:37Z

For example, "fix e2e failure" is meaningful within the context of this PR. But once merged to main, it would become rather ambiguous.

Yes many commits may not be meaningful out of PR. But no matter how many commits are in the PR, in the end there's only one merged commit right? Like this: 128acfc

Of course if that's really necessary I'll do it. But I have seen many merged PRs with several commits, even from maintainers. So currently I don't see any values of doing it here. If I miss something else then please specify.

slashpai · 2024-04-03T05:42:00Z

IMHO for best practices its good to squash commits with meaningful message. Incase not doing GitHub has option squash and merge but we would still need to update comments while doing that.

haanhvu · 2024-04-03T06:03:09Z

IMHO for best practices its good to squash commits with meaningful message. Incase not doing GitHub has option squash and merge but we would still need to update comments while doing that.

Yes, please help me squash when you merge. The first commit message has everything you need to know about the PR. I always follow your format here.

Personally, I prefer to keep PR commits history as they are. In case I do several approaches in one PR, that PR merged, then I need to revert the PR and come back to a previous approach, commits history can help me find back that approach in my PR.

simonpasquier · 2024-04-03T09:42:48Z

pkg/prometheus/testdata/AdditionalAlertRelabelConfigs.golden

+  separator: ""
+  action: replace
+  targetLabel: custom-label


Suggested change

- sourceLabels: [label-a, label-b]

separator: ""

action: replace

targetLabel: custom-label

- source_labels: [label-a, label-b]

separator: ""

action: replace

target_label: custom-label

Thanks. Fixed it. Any ideas why the test still passed with the operator's format? 🤔

I thought the input file followed the operator's format. And the output (expected) file followed Prometheus's format. But from your suggestion, it means both follow Prometheus's format?

correct, it's because the test is about additionalScrapeConfigs which hold native Prometheus configs.

Hmm, then do you think this test can verify our fix? I thought we needed a test regarding the operator's format? 🤔 Like, before the operator's format didn't support "" but now it does?

The operator appends the additional config that is specified by name (I think?) in the final generated config. In this, we process the input config (user defined) and generate a final config. I think what you're trying to test is that the generated config which contains the appended additional config still contains the "empty" value in field instead of omitting it entirely.

Allow empty separator in relabel config. This is corresponding to Prometheus' relabel config. Fixes prometheus-operator#5003

haanhvu · 2024-04-03T10:41:00Z

CI didn't run so I squashed commits to reactivate it. It still hasn't run though... Will wait then.

slashpai · 2024-04-03T12:35:34Z

@simonpasquier Is it good to merge?

haanhvu requested a review from a team as a code owner March 23, 2024 11:31

pull-request-size bot added the size/L label Mar 23, 2024

haanhvu changed the title ~~Allow empty seperator in relabel config~~ WIP: Allow empty seperator in relabel config Mar 23, 2024

pull-request-size bot added size/M and removed size/L labels Mar 23, 2024

haanhvu changed the title ~~WIP: Allow empty seperator in relabel config~~ Allow empty seperator in relabel config Mar 23, 2024

haanhvu force-pushed the issue5003-1 branch 2 times, most recently from 8eeb779 to d366d68 Compare March 25, 2024 14:06

simonpasquier reviewed Mar 25, 2024

View reviewed changes

haanhvu force-pushed the issue5003-1 branch from 6b67448 to 39bdf1c Compare March 25, 2024 17:37

pull-request-size bot added size/S size/M and removed size/M size/S labels Mar 25, 2024

simonpasquier mentioned this pull request Mar 29, 2024

Empty replacement is not possible when Action is Replace #6443

Closed

1 task

mviswanathsai reviewed Mar 31, 2024

View reviewed changes

haanhvu force-pushed the issue5003-1 branch from 5974c24 to 562a20d Compare March 31, 2024 10:26

mviswanathsai reviewed Mar 31, 2024

View reviewed changes

pull-request-size bot added size/S and removed size/M labels Mar 31, 2024

ArthurSens approved these changes Apr 1, 2024

View reviewed changes

ArthurSens reviewed Apr 1, 2024

View reviewed changes

ArthurSens mentioned this pull request Apr 1, 2024

Change suffix of testdata that is used as input to .input #6455

Open

simonpasquier reviewed Apr 2, 2024

View reviewed changes

haanhvu force-pushed the issue5003-1 branch from 8bcaf1e to e071e38 Compare April 2, 2024 13:57

haanhvu changed the title ~~Allow empty seperator in relabel config~~ Allow empty separator in relabel config Apr 3, 2024

simonpasquier reviewed Apr 3, 2024

View reviewed changes

haanhvu force-pushed the issue5003-1 branch 2 times, most recently from d65d126 to 5f107bc Compare April 3, 2024 10:20

simonpasquier approved these changes Apr 3, 2024

View reviewed changes

relabel config: allow empty separator

029b236

Allow empty separator in relabel config. This is corresponding to Prometheus' relabel config. Fixes prometheus-operator#5003

haanhvu force-pushed the issue5003-1 branch from 5f107bc to 029b236 Compare April 3, 2024 10:38

simonpasquier merged commit 48444eb into prometheus-operator:main Apr 3, 2024
17 checks passed

haanhvu mentioned this pull request Apr 10, 2024

chore: cut v0.73.1 #6498

Merged

5 tasks

	Separator string `json:"separator"`
	Separator *string `json:"separator"`

	Separator: &relabel.DefaultRelabelConfig.Separator,
	Separator: ptr.To(relabel.DefaultRelabelConfig.Separator),

Allow empty separator in relabel config #6425

Allow empty separator in relabel config #6425

Conversation

haanhvu commented Mar 23, 2024 • edited by ArthurSens

Description

Type of change

Verification

Changelog entry

Question

haanhvu commented Mar 23, 2024 • edited

slashpai commented Mar 25, 2024

haanhvu commented Mar 25, 2024 • 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

haanhvu commented Mar 25, 2024 • edited

simonpasquier commented Mar 26, 2024

mviswanathsai Mar 31, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArthurSens left a comment • 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

mviswanathsai commented Apr 2, 2024

haanhvu commented Apr 2, 2024

mviswanathsai commented Apr 2, 2024

mviswanathsai commented Apr 2, 2024 • edited

haanhvu commented Apr 2, 2024 • edited

slashpai commented Apr 3, 2024

haanhvu commented Apr 3, 2024 • 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

haanhvu Apr 3, 2024 • edited

Choose a reason for hiding this comment

mviswanathsai Apr 3, 2024 • edited

Choose a reason for hiding this comment

haanhvu commented Apr 3, 2024 • edited

slashpai commented Apr 3, 2024

haanhvu commented Mar 23, 2024 •

edited by ArthurSens

haanhvu commented Mar 23, 2024 •

edited

haanhvu commented Mar 25, 2024 •

edited

haanhvu commented Mar 25, 2024 •

edited

mviswanathsai Mar 31, 2024 •

edited

ArthurSens left a comment •

edited

mviswanathsai commented Apr 2, 2024 •

edited

haanhvu commented Apr 2, 2024 •

edited

haanhvu commented Apr 3, 2024 •

edited

haanhvu Apr 3, 2024 •

edited

mviswanathsai Apr 3, 2024 •

edited

haanhvu commented Apr 3, 2024 •

edited