Fix panic when telemetry metrics are disabled #5642
Conversation
|
As seen in the CI logs the unit test reproduces the crash |
Codecov Report
@@ Coverage Diff @@
## main #5642 +/- ##
==========================================
+ Coverage 91.33% 91.41% +0.08%
==========================================
Files 191 191
Lines 11384 11386 +2
==========================================
+ Hits 10398 10409 +11
+ Misses 786 778 -8
+ Partials 200 199 -1
Continue to review full report at Codecov.
|
| // The process telemetry initialization requires the ballast size, which is available after the extensions are initialized. | ||
| if err = telemetry.RegisterProcessMetrics(srv.telemetryInitializer.ocRegistry, getBallastSize(srv.host)); err != nil { | ||
| return nil, fmt.Errorf("failed to register process metrics: %w", err) | ||
| if set.Config.Telemetry.Metrics.Level != configtelemetry.LevelNone && set.Config.Telemetry.Metrics.Address != "" { |
There was a problem hiding this comment.
I would expect an empty address to return a validation error, instead of silently disabling telemetry. Do you think this is worth an issue?
There was a problem hiding this comment.
👍 I can open an issue for that, I would also expect an empty address to make it fail. (I didn't mention this in the PR but this check comes from
)There was a problem hiding this comment.
Thanks! Yeah I eventually tracked down the same check :D
Description: Fix panic when
telemetry::metrics::levelis set tonone.Link to tracking Issue: Fixes #5641
Testing: Add unit test for avoiding regressions