[chore] reuse the timer object when parsing #31802
Conversation
atoulme
force-pushed
the
reuse_timer_when_parsing
branch
from
609d54d to
1878d83
Compare
pkg/stanza/operator/helper/time.go
Outdated
| if t.LayoutType == "" { | ||
| t.LayoutType = StrptimeKey | ||
| } |
There was a problem hiding this comment.
Why do we need to set default values in the unmarshaller? If we need this one then why not others? Maybe this is just a matter of test cases creating the default config consistently?
There was a problem hiding this comment.
The code was doing this previously in the NewTimeParser path. Instead of creating a new struct, I just set the default here now.
There was a problem hiding this comment.
I don't understand why though. The pattern we use elsewhere is to instantiate a default struct and then unmarshal into it.
There was a problem hiding this comment.
no, that's the only instance this occurs afaict? By patching this code open-telemetry/opentelemetry-collector#9750 works well.
There was a problem hiding this comment.
I think it is a well established pattern throughout the repo to create a default config and then unmarshal into them. For pkg/stanza operators, it is somewhat cryptic but is handled here where we've basically looked up a factory for the specific operator and asked it for a new (default) config. I don't think we should be handling default values here.
There was a problem hiding this comment.
I see, in that case, since the default value is set earlier, we can safely remove the check.
There was a problem hiding this comment.
Looks like that didn't work: the registry works well to set default values, but because TimeParser is optional on helper.ParserConfig, it is nil by default, and since it's not initialized with default values, they don't get applied later on.
There was a problem hiding this comment.
@atoulme Can't we do the trick from https://github.com/open-telemetry/opentelemetry-collector/blob/c72092e00151ce8e4cfef32c73b3a80d54076278/receiver/otlpreceiver/config.go#L68-L70 ? That is, set the configuration, but remove it during unmarshaling if the key is not set
There was a problem hiding this comment.
you are right, that's the right fix. I will try to get to it.
|
We're stuck on #31802 (comment) |
|
Closing in favor of #32568 |
**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.
**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.
Description:
This is a companion PR to handle recursive state of unmarshalers with open-telemetry/opentelemetry-collector#9750.
By reusing the same struct when parsing its contents, we can avoid running into an infinite loop.