open-telemetry / opentelemetry-collector-contrib

Contrib repository for the OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
3.06k stars 2.36k forks source link

[ottl] ParseTime should pre-convert from strptime to gotime format #35078

Closed djaglowski closed 1 month ago

djaglowski commented 1 month ago

Component(s)

pkg/ottl

Describe the issue you're reporting

Currently, the Time function runs timeutils.ParseStrptime for every string that needs to be parsed as a time.Time. This in turn calls ctimefmt.ToNative, which is doing a deterministic string replacement operation.

The OTTL function should instead call timeutils.StrptimeToGotime once at collector startup, and then call timeutils.ParseGotime during normal operations.

djaglowski commented 1 month ago

For context, ToNative was observed to be wasting ~5% of compute on a real world configuration.

image

github-actions[bot] commented 1 month ago

Pinging code owners:

github-actions[bot] commented 1 month ago

Pinging code owners for pkg/ottl: @TylerHelmuth @kentquirk @bogdandrutu @evan-bradley. See Adding Labels via Comments if you do not have permissions to add labels yourself.

TylerHelmuth commented 1 month ago

@djaglowski I'm interested in fixing this performance issue, but I don't see any connection to timeutils.ParseStrptime from ParseJSON.

I only see timeutils.ParseStrptime called from the Time converter. I suspect your proposal could be implemented there.

djaglowski commented 1 month ago

Sorry, I got some wires crossed looking at several issues at once. ParseJSON is not the function in question. It should read Time instead. I've now corrected the wording above.