Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pkg/stanza] TimeParser unmarshaling ignores original configuration #32169

Closed
mx-psi opened this issue Apr 4, 2024 · 4 comments · Fixed by #32568
Closed

[pkg/stanza] TimeParser unmarshaling ignores original configuration #32169

mx-psi opened this issue Apr 4, 2024 · 4 comments · Fixed by #32568
Labels
bug Something isn't working pkg/stanza priority:p3 Lowest

Comments

@mx-psi
Copy link
Member

mx-psi commented Apr 4, 2024

Component(s)

pkg/stanza

What happened?

Description

The TimeParser struct seems to ignore the original configuration if the struct was initialized with something other than the default values:

func (t *TimeParser) Unmarshal(component *confmap.Conf) error {
cfg := NewTimeParser()
err := component.Unmarshal(&cfg, confmap.WithIgnoreUnused())
if err != nil {
return err
}
*t = cfg
return nil
}

Steps to Reproduce

This test reproduces it

func TestUnmarshal(t *testing.T) {
	conf := confmap.NewFromStringMap(map[string]any{
		"location": "America/Shiprock",
	})
	tp := TimeParser{
		Layout: "1/2/2006 15:04:05",
	}

	require.NoError(t, tp.Unmarshal(conf))
	assert.Equal(t, "America/Shiprock", tp.Location) // ok
	assert.Equal(t, "strptime", tp.LayoutType)           // ok, although weird
	assert.Equal(t, "1/2/2006 15:04:05", tp.Layout)  // FAIL --> empty
}

Expected Result

All assertions pass

Actual Result

The last assertion fails

Collector version

Tested as of b25b78a

Environment information

No response

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

I think NewTimeParser should be part of the default configuration of time parser instead of this, and we should do the 'set to nil' trick from https://github.com/open-telemetry/opentelemetry-collector/blob/c72092e00151ce8e4cfef32c73b3a80d54076278/receiver/otlpreceiver/config.go#L68-L70 if we want to only set default config if the key exists.

@mx-psi mx-psi added the bug Something isn't working label Apr 4, 2024
Copy link
Contributor

github-actions bot commented Apr 4, 2024

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@mx-psi
Copy link
Member Author

mx-psi commented Apr 4, 2024

@mx-psi mx-psi added the priority:p3 Lowest label Apr 4, 2024
@atoulme
Copy link
Contributor

atoulme commented Apr 5, 2024

Agreed that this can be considered a bug.

@atoulme
Copy link
Contributor

atoulme commented Apr 5, 2024

I believe #31802 addresses this bug.

djaglowski pushed a commit that referenced this issue Apr 20, 2024
**Description:**
Fix unmarshaling of TimeParser to conserve initial settings before
unmarshaling.

**Link to tracking Issue:**
Fixes #32169

**Testing:**
Added the test from the issue.

For @mx-psi I could not make it so we would apply
#31802 (comment)
because `TimeParser` is reused on all operators, and therefore it
becomes contentious to patch all over the place.
LokeshOpsramp pushed a commit to opsramp/opentelemetry-collector-contrib that referenced this issue May 5, 2024
**Description:**
Fix unmarshaling of TimeParser to conserve initial settings before
unmarshaling.

**Link to tracking Issue:**
Fixes open-telemetry#32169

**Testing:**
Added the test from the issue.

For @mx-psi I could not make it so we would apply
open-telemetry#31802 (comment)
because `TimeParser` is reused on all operators, and therefore it
becomes contentious to patch all over the place.
rimitchell pushed a commit to rimitchell/opentelemetry-collector-contrib that referenced this issue May 8, 2024
**Description:**
Fix unmarshaling of TimeParser to conserve initial settings before
unmarshaling.

**Link to tracking Issue:**
Fixes open-telemetry#32169

**Testing:**
Added the test from the issue.

For @mx-psi I could not make it so we would apply
open-telemetry#31802 (comment)
because `TimeParser` is reused on all operators, and therefore it
becomes contentious to patch all over the place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg/stanza priority:p3 Lowest
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants