open-telemetry / opentelemetry-collector

OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
3.98k stars 1.33k forks source link

Memoize the result of Resolve to avoid duplicate read operations to the config #9982

Open ms-hujia opened 1 month ago

ms-hujia commented 1 month ago

Description:

For the implementation of ConfigProvider, the result of Resolve is memoized to avoid duplicate read operations to the config. The benefits include:

  1. It could optimize the initialization of OTEL Collector. For Collector, the existing code will perform another extra read operation to the config right after the first one.
  2. It could avoid missing change notification from Watch() in some rare corner case. This could happen after the watch is tear down and before new watch is setup in the second invocation to Resolve().

Link to tracking Issue: N/A.

Testing: No change to the interface and behavior, so all existing tests should be enough.

Documentation: N/A.

linux-foundation-easycla[bot] commented 1 month ago

CLA Signed

The committers listed above are authorized under a signed CLA.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 91.54%. Comparing base (fb9d80d) to head (d6696fb). Report is 83 commits behind head on main.

Files Patch % Lines
otelcol/collector.go 60.00% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #9982 +/- ## ========================================== - Coverage 91.54% 91.54% -0.01% ========================================== Files 360 360 Lines 16695 16694 -1 ========================================== - Hits 15284 15283 -1 Misses 1074 1074 Partials 337 337 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ms-hujia commented 1 month ago

I am curious if any performance gains are worth this extra complexity. Could you provide some benchmarks?

As I tested, the performance difference is very small (<0.05s) in benchmark. Unless I use a giant config file or emulate by a web server with delay, the improvement would be great.

As I explained in description, the point here is mainly avoiding a duplicate read operation in the same function of collector.

ms-hujia commented 4 weeks ago

Ping for review... @TylerHelmuth @mx-psi @atoulme

github-actions[bot] commented 2 weeks ago

This PR was marked stale due to lack of activity. It will be closed in 14 days.