add tbs storage limit and disk-related metrics#19568
Conversation
🤖 GitHub commentsJust comment with:
|
|
This pull request does not have a backport label. Could you fix it @rubvs? 🙏
|
9eb164d to
082a2e7
Compare
💚 Build Succeeded
History
|
|
I also ran the test following the steps posted by Ruben, and can confirm that the metrics mapping appears. |
carsonip
left a comment
There was a problem hiding this comment.
implementation looks good, 1 nit on metric naming
| sm.storageMetrics.storageLimitGauge, _ = meter.Int64Gauge("apm-server.sampling.tail.storage.storage_limit") | ||
| sm.storageMetrics.diskUsedGauge, _ = meter.Int64Gauge("apm-server.sampling.tail.storage.disk_used") | ||
| sm.storageMetrics.diskTotalGauge, _ = meter.Int64Gauge("apm-server.sampling.tail.storage.disk_total") | ||
| sm.storageMetrics.diskUsageThresholdGauge, _ = meter.Int64Gauge("apm-server.sampling.tail.storage.disk_usage_threshold") |
There was a problem hiding this comment.
nit: I wonder if it will be more descriptive / self-explanatory with a suffix e.g. _pct. Also wonder if it better to range from 0 to 1 (correct to 2 d.p. maybe?) instead of 0-100.
| if sm.storageMetrics.diskTotalGauge != nil { | ||
| sm.storageMetrics.diskTotalGauge.Record(context.Background(), int64(usage.TotalBytes)) | ||
| } | ||
| // Record disk usage threshold as a percentage (0-100) |
There was a problem hiding this comment.
nit: remember to update comment if we change to 0-1
| func (sm *StorageManager) NewReadWriter(storageLimit uint64, diskUsageThreshold float64) RW { | ||
| // Store configured values for monitoring metrics | ||
| sm.configuredStorageLimit.Store(storageLimit) | ||
| // Store disk usage threshold as percentage (0-100) |
5b72685 to
1a1df2e
Compare
There was a problem hiding this comment.
thanks!
For backports,
- this obviously cannot be directly backported to 8.x as is due to the 9.0-introduced threshold, so expect some changes there.
- Backport active 9 is fair, but bear in mind that also means ideally (not necessarily) all relevant metric PRs in ES, beats, integration need to be backported to the same patch version. It will be a bit of work. Alternatively, you may backport to 9.3 only, or no backport but the change will sit in main for a few months.
|
Pending on the other PRs to pass CI and be approved first. |
4ea3b35 to
6f17f3a
Compare
|
Force pushed to fix some CLA issues. The contents are the same @carsonip, though it seems like system-test-fips failed again just now. |
0faa752 to
7036d0f
Compare
|
|
Motivation/summary
Expose a new set of metrics to enhance TBS observability. The metric fields in the index data are tested in this PR, while the mappings are tested in the corresponding linked PRs. Changes in the elastic/elasticsearch#138131 PR are tested in conjunction with the changed in this PR.
See #15533 (comment) for the detailed overview.
Depends on PR:
Checklist
For functional changes, consider:
How to test these changes
Step 1: Ensure Elasticsearch & Kibana is running
Depends on elastic/elasticsearch#138131 with updates to
monitoring-beats.json.apm-server/docker-compose.ymland change the ES image.Step 2: Create APM Server config
Step 3: Start APM Server
Run APM Server binary directly:
Step 6: Verify Data
Verify data in index
.monitoring-beats-7-*Verify mappings for index
monitoring-beats-7-*Related issues
Part of #15533