feat: add support for RDB current file size metric#1095
feat: add support for RDB current file size metric#1095akshaykumar-vijapur wants to merge 7 commits intooliver006:masterfrom
Conversation
oliver006
left a comment
There was a problem hiding this comment.
This is great, thanks for opening this PR!
I'll give it a more thorough review later.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1095 +/- ##
==========================================
- Coverage 81.88% 81.75% -0.14%
==========================================
Files 20 21 +1
Lines 2700 2752 +52
==========================================
+ Hits 2211 2250 +39
- Misses 370 381 +11
- Partials 119 121 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks for the PR - this feature is useful.
I left a bunch of comments that need addressing before we can move forward here.
// Made with Bob
It's ok to submit PRs with AI-generated code, I use AI myself all the time.
But I'd still expect a higher bar for code quality and expect you to review your own code before submitting the PR.
And/or use a better AI model.
6d94502 to
a4cdc84
Compare
Coverage Report for CI Build 23999617636Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.09%) to 85.577%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
9b35a07 to
2296cc8
Compare
2296cc8 to
11d06b1
Compare
|
@oliver006 Pls review and check. |
|
@oliver006 Pls check as we need this metrics in our env |
- Fix TestExtractRdbFileSizeMetricConfigDisabled to use e.Collect(ch) pattern - Refactor extractRdbFileSizeMetric to accept config array instead of redis.Conn - Eliminates redundant CONFIG GET call by reusing already-extracted config - Parses config array to extract 'dir' and 'dbfilename' values - Move rdb_current_size_bytes metric description between number_of_distinct_key_groups and script_result - Update TestExtractRdbFileSizeMetric to use e.Collect(ch) pattern Addresses review feedback from oliver006
|
@oliver006 pls review |
|
@oliver006 pls review |
## End-to-End Test (Inside Container)
### Setup:
```bash
# Start Redis container with exposed ports
docker run -d --name redis-e2e-final -p 6381:6379 -p 9125:9125 redis:latest
# Create test data and RDB file
docker exec redis-e2e-final redis-cli SET key1 "value1"
docker exec redis-e2e-final redis-cli SET key2 "value2"
docker exec redis-e2e-final redis-cli BGSAVE
sleep 2Output:Verify RDB File:docker exec redis-e2e-final ls -lh /data/dump.rdbOutput:RDB File Size: 119 bytes Deploy Exporter Inside Container:# Copy linux binary to container
docker cp ./redis_exporter_linux redis-e2e-final:/usr/local/bin/redis_exporter
docker exec redis-e2e-final chmod +x /usr/local/bin/redis_exporter
# Start exporter with RDB metric enabled
docker exec -d redis-e2e-final /usr/local/bin/redis_exporter \
--redis.addr=redis://localhost:6379 \
--include-rdb-file-size-metric \
--web.listen-address=:9125 \
--log-format=txt
sleep 5Fetch Metrics:curl -s http://localhost:9125/metrics | grep -B1 -A1 "rdb_current_size_bytes"Output:Verification:docker exec redis-e2e-final stat -c %s /data/dump.rdbOutput:Result: Metric accurately reports RDB file size |
| } | ||
| } | ||
|
|
||
| if e.options.ConfigCommandName != "-" && e.options.InclRdbFileSizeMetric && len(config) > 0 { |
There was a problem hiding this comment.
remove the extra len(config) > 0 check here - extractRdbFileSizeMetric() handles that
| } | ||
|
|
||
| dbCount := 0 | ||
| var config []interface{} |
There was a problem hiding this comment.
instead of the extra variable, call extractRdbFileSizeMetric() right up here, after line 839
| log "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| func (e *Exporter) extractRdbFileSizeMetric(ch chan<- prometheus.Metric, config []interface{}) { |
There was a problem hiding this comment.
I just simplified the config extraction anfd it's using a ma[string]string now which should make this code a lot simpler
for instance,m you don't need to iterate over the config map any longer but can simply look up filename and directory.
Please rebase and then use the config map.
Test result
All RDB-related Metrics
Output:
closes #1093