[2.10] [MOD-12070] Extend indexing metrics (#7669)#7766
Conversation
* Total num indexed metric * total docs indexed by field type * test total indexed metic * test field metric * format tests * remove empty line * remove unecessary wait * meirav comments * more comments + change metric name * expose and test geometry * Unify metric * test multi json (cherry picked from commit b346977)
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| RSGlobalStats.fieldsStats.geometryTotalDocsIndexed += toAdd; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Global stats update is not thread-safe
FieldsGlobalStats_UpdateFieldDocsIndexed updates RSGlobalStats.fieldsStats.*TotalDocsIndexed with non-atomic writes based on an assumption that indexing runs only on the main thread / with the GIL. It’s invoked from fulltextPreprocessor, which is documented as running with the GIL released, so concurrent indexing can race and lose increments or cause undefined behavior.
Additional Locations (1)
| RSGlobalStats.fieldsStats.geometryTotalDocsIndexed += toAdd; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Multi-type fields never increment indexing counters
FieldsGlobalStats_UpdateFieldDocsIndexed uses switch (fs->types) as if fs->types is a single value, but it’s a bitmask and can represent multiple types (especially for FieldSpec_Dynamic). In those cases no case matches, so the new total_indexing_ops_* metrics silently stop counting for multi-type fields.
Additional Locations (1)
| // Since we are here, the indexing was successful, update the global statistics. | ||
| FieldsGlobalStats_UpdateFieldDocsIndexed(fs, 1); | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
Bug: Metrics counted even when add fails
fulltextPreprocessor increments FieldsGlobalStats_UpdateFieldDocsIndexed() before Document_AddToIndexes() knows the document was successfully committed (e.g., if Indexer_Add() fails later). This can permanently inflate total_indexing_ops_text_fields even though the document wasn’t actually indexed.
There was a problem hiding this comment.
Why didn't it bother him in the previous pr?
There was a problem hiding this comment.
and remind me why wasnt the metric placed here?
Line 839 in fd1b5c2
There was a problem hiding this comment.
Note that text fields are returned immediately in Indexer_Add() -> IndexerProcess()
if (ACTX_IS_INDEXED(aCtx) || aCtx->stateFlags & (ACTX_F_ERRORED)) { // Document is complete or errored. No need for further processing. if (!(aCtx->stateFlags & ACTX_F_EMPTY)) { return; } }
| RSGlobalStats.fieldsStats.geometryTotalDocsIndexed += toAdd; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Negative updates underflow size_t counters
The function documents that toAdd “can be negative”, but it adds int toAdd into size_t fields like RSGlobalStats.fieldsStats.textTotalDocsIndexed. Any negative call will wrap/underflow and massively corrupt the counters.
meiravgri
left a comment
meiravgri
left a comment
There was a problem hiding this comment.
small comments referencing cursor's concerns
| // Since we are here, the indexing was successful, update the global statistics. | ||
| FieldsGlobalStats_UpdateFieldDocsIndexed(fs, 1); | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
Why didn't it bother him in the previous pr?
| // Since we are here, the indexing was successful, update the global statistics. | ||
| FieldsGlobalStats_UpdateFieldDocsIndexed(fs, 1); | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
and remind me why wasnt the metric placed here?
Line 839 in fd1b5c2
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 2.10 #7766 +/- ##
==========================================
- Coverage 89.41% 89.41% -0.01%
==========================================
Files 210 210
Lines 36138 36177 +39
==========================================
+ Hits 32314 32346 +32
- Misses 3824 3831 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* [MOD-12070] Extend indexing metrics (#7669) * Total num indexed metric * total docs indexed by field type * test total indexed metic * test field metric * format tests * remove empty line * remove unecessary wait * meirav comments * more comments + change metric name * expose and test geometry * Unify metric * test multi json (cherry picked from commit b346977) * remove no json * remove total indexing time (cherry picked from commit 8ea201f)
|
Successfully created backport PR for |
[2.10] [MOD-12070] Extend indexing metrics (#7669) (#7766) * [MOD-12070] Extend indexing metrics (#7669) * Total num indexed metric * total docs indexed by field type * test total indexed metic * test field metric * format tests * remove empty line * remove unecessary wait * meirav comments * more comments + change metric name * expose and test geometry * Unify metric * test multi json (cherry picked from commit b346977) * remove no json * remove total indexing time (cherry picked from commit 8ea201f) Co-authored-by: lerman25 <[email protected]>
2 participants
backport #7669 to 2.10
Note
Adds global counters for total documents indexed (overall and per field type) and surfaces them in INFO MODULES, updating indexing paths to maintain these stats with tests.
FieldsGlobalStats_UpdateFieldDocsIndexedand new counters inFieldsGlobalStatsfor per-field-type document indexing totals.src/document.c(fulltextPreprocessor,IndexerBulkAdd).total_num_docs_in_indexesacross all specs inIndexesInfo_TotalInfoand expose in index stats.total_indexing_ops_{text|tag|numeric|geo|geoshape|vector}_fieldsandtotal_num_docs_in_indexesinsrc/info/info_redis.c.global_stats.h,indexes_info.h) and includes to support new metrics.total_num_docs_in_indexesand per-fieldtotal_indexing_ops_*counters, including multi-value JSON behavior.Written by Cursor Bugbot for commit 2204869. This will update automatically on new commits. Configure here.