-
Notifications
You must be signed in to change notification settings - Fork 932
Feature: Add Redis search module indexes metrics collection #1046
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: Add Redis search module indexes metrics collection #1046
Conversation
3c143bb to
6fc64d7
Compare
|
So far in draft mode as unfinished. I believe I hit limitations of |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1046 +/- ##
==========================================
- Coverage 81.96% 81.93% -0.04%
==========================================
Files 19 20 +1
Lines 3017 3094 +77
==========================================
+ Hits 2473 2535 +62
- Misses 431 443 +12
- Partials 113 116 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pull Request Test Coverage Report for Build 18565399734Details
💛 - Coveralls |
b9e61f4 to
24fd8c5
Compare
|
Implemented metrics collection, descriptions are taken from FT.INFO page, example: |
24fd8c5 to
fdcd4ec
Compare
exporter/exporter.go
Outdated
| "search_index_total_index_memory_size_bytes": {txt: "Total memory used by search index", lbls: []string{"index_name"}}, | ||
| "search_index_geoshapes_size_bytes": {txt: "Memory used by GEO-related fields", lbls: []string{"index_name"}}, | ||
| "search_index_records_per_doc_avg": {txt: "Average number of records (including deletions) per document", lbls: []string{"index_name"}}, | ||
| "search_index_bytes_per_record_avg": {txt: "Average size of each record in bytes", lbls: []string{"index_name"}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I converted other metrics from megabytes, _mb to _bytes and keep _bytes in the end according to Prometheus metrics naming schema.
However, for this metric I think it's worth to keep naming as is, since it's pre-aggregated value from redis and there're 4 metrics in total with similar naming, like search_index_records_per_doc_avg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could go either way about the pre-aggregated ones. in theory they can be calculated on the prometheus side (plus avg is only so useful) - if you want to just drop them then I
'm fine too
one thing re naming is, unit should go to the end so sohuld be something like search_index_avg_per_record_bytes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth to keep these avg metrics just in case as I'm not sure atm about their usefulness. But it's only 4 metrics per index.
Regarding renaming. This will break naming schema for these 4 metric - now they all end with _avg. Though doesn't look like a big deal to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think it's worth to rename all 4 of them a little bit, like
- search_index_records_per_doc_avg -> search_index_avg_per_doc_records
- search_index_bytes_per_record_avg -> search_index_avg_per_record_bytes
- search_index_offsets_per_term_avg -> search_index_avg_per_term_offsets
- search_index_offset_bits_per_record_avg -> search_index_avg_per_record_offset_bits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
exporter/search_indexes.go
Outdated
| log "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| // All fields of the streamInfo struct must be exported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this header similar to exporter/streams.go file, I guess limitation is the same, since I'm using redis.ScanStruct here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: this should be searchIndexInfo and not streamInfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for noticing, fixed now.
0083872 to
731eced
Compare
9c629a7 to
687673a
Compare
| log "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| func setupSearchIndex(t *testing.T, addr string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added helper function, similar to setupKeys() in exporter/exporter_test.go to create index in empty redis.
Otherwise there's nothing and this test scenario fails because no search_index metrics collected: {addr: os.Getenv("TEST_REDIS8_URI"), inclSearchIndexesMetrics: true, wantSearchIndexesMetrics: true}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
@oliver006 PTAL when you have time, I think PR is ready for review. |
* Add Redis search module indexes metrics collection * Add tests for Redis search module indexes metrics collection * Bump redigo to v1.9.3 due to gomodule/redigo#691 Fix: oliver006#1043
687673a to
707a441
Compare
|
Thanks for the PR - I'll try to get to reviewing it later this week |
…etrics-collection
|
PR looks good - only a few minor questions or comments. |
6d6f6b4 to
499d6b1
Compare
499d6b1 to
542dd15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you!
|
RFM? |
oliver006
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Fix: #1043