[backport 8.19] add tbs storage limit metrics#20682
Conversation
🤖 GitHub commentsJust comment with:
|
ericywl
changed the title
x-pack/apm-server/main_test.go
Outdated
| storageLimit := getGauge(t, reader, "apm-server.sampling.tail.storage.storage_limit") | ||
| assert.Zero(t, storageLimit) |
There was a problem hiding this comment.
nit: the test might give better confidence if storage limit is non-zero. Also non-zero tests the *0.9 quirk
There was a problem hiding this comment.
Pull request overview
Backports tail-based sampling (TBS) storage-limit observability to the 8.19 APM Server by adding a new storage_limit metric and consolidating TBS storage metric registration within the Badger storage manager.
Changes:
- Add an opt-in OpenTelemetry
MeterProvidertoeventstorage.StorageManagerand register TBS storage metrics there (including newapm-server.sampling.tail.storage.storage_limit). - Move metric callback registration/unregistration responsibility from
main.gointoStorageManagerlifecycle (NewStorageManager/Close), and record the effective storage limit inRun. - Update monitoring tests to assert the new metric and loosen metric assertions to “contains” to avoid brittleness.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| x-pack/apm-server/sampling/eventstorage/storage_manager.go | Adds MeterProvider-based metric instruments/callback registration, records effective storage limit, and unregisters callback on close. |
| x-pack/apm-server/main.go | Passes MeterProvider into NewStorageManager via an option and removes the previous global metric callback registration/unregistration. |
| x-pack/apm-server/main_test.go | Updates monitoring assertions to “contain” and adds coverage for the new storage_limit metric. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| type StorageManagerOptions func(*StorageManager) | ||
|
|
||
| func WithMeterProvider(mp metric.MeterProvider) StorageManagerOptions { | ||
| return func(sm *StorageManager) { | ||
| sm.meterProvider = mp | ||
| } | ||
| } |
There was a problem hiding this comment.
StorageManagerOptions is a functional-option type but is named in plural form. The prevailing pattern in this repo uses singular ...Option for these (e.g. LoadConfigOption in internal/beatcmd/config.go and ConfigOption in internal/telemetry/metric_exporter_config.go). Consider renaming this to StorageManagerOption and updating NewStorageManager to accept ...StorageManagerOption for consistency.
There was a problem hiding this comment.
Let's keep it as StorageManagerOptions to be consistent with main. It was a typo in retrospect. Alternatively, fix it in both 9.x and 8.19
💚 Build Succeeded
History
cc @ericywl |
Closes #20531.
This is a manual backport of #20464. Due to differences in TBS implementation between 8.19 and 9.x, the disk-related metrics are not backported.
Related PRs: