configuration: Implement IsZero for relabel.Regex to remove default regex #14004

liam-howe-maersk · 2024-04-30T09:03:32Z

Currently, when unmarshalling some relabel config from YAML, if the regex field is not provided we set this to be equal to the default value. If we then want to marshal this config back in to YAML, it will generate the YAML with the regex field set equal to the default YAML value, meaning that unmarshalling and then marshalling again doesn't give the same output as the original input. For the keepequal and dropequal actions we expect that no regex field is set and therefore an error is returned if we try to unmarshal, then marshal and again unmarshal one of these actions, because the regex field will be set on the marshal step. By implementing the MarshalYAML function and ensuring the regex field is not set if it was not provided originally, we fix this bug.

… regex field if it was not provided in the first place Signed-off-by: Liam Howe <liam.howe@maersk.com>

bboreham · 2024-04-30T11:15:10Z

This seems workable, but I was rather confused for a while. I was unfamiliar with the MarshalYAML method, which is not expected to do any marshalling, but to return an object based on its input which is then marshalled.

Having got that deep into the workings, I discovered an alternative approach: since the field is declared as omitempty we can explain that the default regexp should be taken as empty. What do you think?

diff --git model/relabel/relabel.go model/relabel/relabel.go
index d29c3d07a..b92b4e903 100644
--- model/relabel/relabel.go
+++ model/relabel/relabel.go
@@ -205,6 +205,11 @@ func (re Regexp) MarshalYAML() (interface{}, error) {
        return nil, nil
 }
 
+// MarshalYAML implements the yaml.IsZeroer interface.
+func (re Regexp) IsZero() bool {
+       return re.Regexp == DefaultRelabelConfig.Regex.Regexp
+}
+
 // String returns the original string used to compile the regular expression.
 func (re Regexp) String() string {
        str := re.Regexp.String()

Signed-off-by: Liam Howe <liam.howe@maersk.com>

liam-howe-maersk · 2024-04-30T14:47:26Z

@bboreham Thanks for reviewing, this is good to know and I agree that the IsZero implementation is a cleaner approach, I have updated the PR as suggested!

bboreham

lgtm

liam-howe-maersk · 2024-05-03T11:37:33Z

lgtm

Thanks for the review @bboreham! I don't have permissions to merge, will this PR be merged by someone else?

bboreham · 2024-05-03T12:56:11Z

Yes I can merge, was just waiting to see if anyone else had a view.

machine424

lgtm, thanks.

Not related to the PR:
Even though we have a default regex, I don't see why we fallback on the empty regex here

prometheus/model/relabel/relabel.go

Lines 102 to 110 in 4b7a44c

    
           func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error { 
        
           	*c = DefaultRelabelConfig 
        
           	type plain Config 
        
           	if err := unmarshal((*plain)(c)); err != nil { 
        
           		return err 
        
           	} 
        
           	if c.Regex.Regexp == nil { 
        
           		c.Regex = MustNewRegexp("") 
        
           	}

Also empty regex is marshalled differently

prometheus/model/relabel/relabel.go

Lines 201 to 206 in 4b7a44c

    
           func (re Regexp) MarshalYAML() (interface{}, error) { 
        
           	if re.String() != "" { 
        
           		return re.String(), nil 
        
           	} 
        
           	return nil, nil 
        
           }

, we kind of have the same issue: "user provides empty regex and after unmarshal/marshal, they'll get null"

machine424 · 2024-05-06T11:18:28Z

model/relabel/relabel.go

@@ -205,6 +205,11 @@ func (re Regexp) MarshalYAML() (interface{}, error) {
 	return nil, nil
 }

+// IsZero implements the yaml.IsZeroer interface.
+func (re Regexp) IsZero() bool {
+	return re.Regexp == DefaultRelabelConfig.Regex.Regexp


I assume this takes into account cases where the default (.*) regex is explicitly provided, as we compare pointers here.
It'd be great to have a regression test for it though.

Good suggestion, I have added a test case for the default regex being explicitly provided, thanks.

The test shows that this change is not good... Because the input and the marshalled should be different then

I'm not sure I follow, if the regex is not provided in the original yaml, then when we unmarshal and marshal again it will still not be in the resulting yaml. If the default regex is explicitly provided then when we unmarshal and marshal it will still be there explicitly in the yaml, is that not expected?

Signed-off-by: Liam Howe <liam.howe@maersk.com>

machine424

lgtm.

roidelapluie · 2024-05-16T10:18:00Z

I think the last test is wrong and should error. We might just call String() to compare the regexes.

bboreham · 2024-06-19T10:18:43Z

@roidelapluie the whole idea of this PR is to avoid comparing the regexp as a string, since "never set" and "set to a string equal to the default" are not the same.

beorn7 · 2024-06-26T13:21:00Z

I tried to reach @roidelapluie out of band. Let's wait for another day or so if he comes back to us with an explanation. Otherwise, we have to assume @bboreham and @machine424 are right and this is the way to go.

roidelapluie · 2024-06-27T10:06:37Z

OK, let's move forward with this.

Implement MarshalYAML for relabel.Config so that we do not generate a…

835dfa7

… regex field if it was not provided in the first place Signed-off-by: Liam Howe <liam.howe@maersk.com>

liam-howe-maersk mentioned this pull request Apr 30, 2024

KeepEqual/DropEqual should compare Regex.String instead of instance values #12534

Closed

Replace MarshalYAML() with IsZero() method for Regexp

5d97f17

Signed-off-by: Liam Howe <liam.howe@maersk.com>

bboreham changed the title ~~configuration: Implement MarshalYAML for relabel.Config to remove default regex~~ configuration: Implement IsZero for relabel.Regex to remove default regex Apr 30, 2024

bboreham previously approved these changes Apr 30, 2024

View reviewed changes

machine424 reviewed May 6, 2024

View reviewed changes

Add test case for default regex explicitly provided

35d897c

Signed-off-by: Liam Howe <liam.howe@maersk.com>

machine424 approved these changes May 7, 2024

View reviewed changes

liam-howe-maersk dismissed bboreham’s stale review via 35d897c June 14, 2024 15:21

beorn7 mentioned this pull request Jun 18, 2024

Do not compare relabel.Regexp as a plain struct #14293

Closed

beorn7 mentioned this pull request Jun 26, 2024

relabel: fix unmarshal #12527

Closed

beorn7 merged commit cb73061 into prometheus:main Jun 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

configuration: Implement IsZero for relabel.Regex to remove default regex #14004

configuration: Implement IsZero for relabel.Regex to remove default regex #14004

Uh oh!

liam-howe-maersk commented Apr 30, 2024

Uh oh!

bboreham commented Apr 30, 2024

Uh oh!

liam-howe-maersk commented Apr 30, 2024

Uh oh!

bboreham left a comment

Uh oh!

liam-howe-maersk commented May 3, 2024

Uh oh!

bboreham commented May 3, 2024

Uh oh!

machine424 left a comment •

edited

Loading

Uh oh!

machine424 May 6, 2024

Uh oh!

liam-howe-maersk May 6, 2024

Uh oh!

roidelapluie May 16, 2024

Uh oh!

liam-howe-maersk May 16, 2024

Uh oh!

machine424 left a comment

Uh oh!

roidelapluie commented May 16, 2024

Uh oh!

bboreham commented Jun 19, 2024

Uh oh!

beorn7 commented Jun 26, 2024

Uh oh!

roidelapluie commented Jun 27, 2024

Uh oh!

Uh oh!

	func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
	*c = DefaultRelabelConfig
	type plain Config
	if err := unmarshal((*plain)(c)); err != nil {
	return err
	}
	if c.Regex.Regexp == nil {
	c.Regex = MustNewRegexp("")
	}

	func (re Regexp) MarshalYAML() (interface{}, error) {
	if re.String() != "" {
	return re.String(), nil
	}
	return nil, nil
	}

configuration: Implement IsZero for relabel.Regex to remove default regex #14004

configuration: Implement IsZero for relabel.Regex to remove default regex #14004

Uh oh!

Conversation

liam-howe-maersk commented Apr 30, 2024

Uh oh!

bboreham commented Apr 30, 2024

Uh oh!

liam-howe-maersk commented Apr 30, 2024

Uh oh!

bboreham left a comment

Choose a reason for hiding this comment

Uh oh!

liam-howe-maersk commented May 3, 2024

Uh oh!

bboreham commented May 3, 2024

Uh oh!

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

machine424 May 6, 2024

Choose a reason for hiding this comment

Uh oh!

liam-howe-maersk May 6, 2024

Choose a reason for hiding this comment

Uh oh!

roidelapluie May 16, 2024

Choose a reason for hiding this comment

Uh oh!

liam-howe-maersk May 16, 2024

Choose a reason for hiding this comment

Uh oh!

machine424 left a comment

Choose a reason for hiding this comment

Uh oh!

roidelapluie commented May 16, 2024

Uh oh!

bboreham commented Jun 19, 2024

Uh oh!

beorn7 commented Jun 26, 2024

Uh oh!

roidelapluie commented Jun 27, 2024

Uh oh!

Uh oh!

machine424 left a comment •

edited

Loading