[RED-169833] fix: fix reporting used memory as unsigned long long#6971
[RED-169833] fix: fix reporting used memory as unsigned long long#6971
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
JoanFM
force-pushed
the
joan-fix-change-info-report
branch
from
817e3c4 to
3860f8b
Compare
JoanFM
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6971 +/- ##
==========================================
- Coverage 85.23% 85.22% -0.01%
==========================================
Files 341 341
Lines 52603 52603
Branches 12526 12526
==========================================
- Hits 44835 44832 -3
- Misses 7576 7579 +3
Partials 192 192
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:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ix-change-info-report
…ix-change-info-report
JoanFM
force-pushed
the
joan-fix-change-info-report
branch
from
c6ccd18 to
701ceda
Compare
| // Total | ||
| RedisModule_InfoAddFieldDouble(ctx, "used_memory_indexes", total_info->total_mem); | ||
| // Total | ||
| RedisModule_InfoAddFieldULongLong(ctx, "used_memory_indexes", total_info->total_mem); |
There was a problem hiding this comment.
These changes are backward compatible, right? (users were getting double values and now they would get unsigned long long, so should not break anything)
There was a problem hiding this comment.
In Redis these are returned as formatted strings, so they should all be properly parsed by a client application
There was a problem hiding this comment.
/* See RedisModule_InfoAddFieldString(). */
int RM_InfoAddFieldULongLong(RedisModuleInfoCtx *ctx, const char *field, unsigned long long value) {
if (!ctx->in_section)
return REDISMODULE_ERR;
if (ctx->in_dict_field) {
ctx->info = sdscatfmt(ctx->info,
"%s=%U,",
field,
value);
return REDISMODULE_OK;
}
ctx->info = sdscatfmt(ctx->info,
"%s_%s:%U\r\n",
ctx->module->name,
field,
value);
return REDISMODULE_OK;
}int RM_InfoAddFieldDouble(RedisModuleInfoCtx *ctx, const char *field, double value) {
if (!ctx->in_section)
return REDISMODULE_ERR;
if (ctx->in_dict_field) {
ctx->info = sdscatprintf(ctx->info,
"%s=%.17g,",
field,
value);
return REDISMODULE_OK;
}
ctx->info = sdscatprintf(ctx->info,
"%s_%s:%.17g\r\n",
ctx->module->name,
field,
value);
return REDISMODULE_OK;
}There was a problem hiding this comment.
And if parsed as double, would still succeed with parsing a string of an unsigned long long, except maybe for the supported range, which could be larger now?
| // Vector memory | ||
| RedisModule_InfoAddFieldDouble(ctx, "used_memory_vector_index", total_info->fields_stats.total_vector_idx_mem); | ||
| // Vector memory | ||
| RedisModule_InfoAddFieldULongLong(ctx, "used_memory_vector_index", total_info->fields_stats.total_vector_idx_mem); |
There was a problem hiding this comment.
This is not documented as bytes
search_used_memory_vector_index: The total memory usage of all vector indexes in the shard.
@JoanFM is it in bytes?
@dwdougherty Shouldn't we make it clearer?
There was a problem hiding this comment.
Will check if is bytes, but IMO this does not affect at all the code. The Field API should be used according to the type declared.
There was a problem hiding this comment.
Looking at the code, it indeed seems to come from bytes.
| RedisModule_InfoAddFieldDouble(ctx, "errors_for_index_with_max_failures", total_info->max_indexing_failures); | ||
| RedisModule_InfoAddFieldDouble(ctx, "OOM_indexing_failures_indexes_count", total_info->background_indexing_failures_OOM); | ||
| RedisModule_InfoAddFieldULongLong(ctx, "errors_for_index_with_max_failures", total_info->max_indexing_failures); | ||
| RedisModule_InfoAddFieldULongLong(ctx, "OOM_indexing_failures_indexes_count", total_info->background_indexing_failures_OOM); |
There was a problem hiding this comment.
@dwdougherty This is not documented (#5778)
@lerman25 Can add description?
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.8
git worktree add -d .worktree/backport-6971-to-2.8 origin/2.8
cd .worktree/backport-6971-to-2.8
git switch --create backport-6971-to-2.8
git cherry-pick -x ca5b662612a4eaeab391f89bcc58f010b3a90dd8 |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.6
git worktree add -d .worktree/backport-6971-to-2.6 origin/2.6
cd .worktree/backport-6971-to-2.6
git switch --create backport-6971-to-2.6
git cherry-pick -x ca5b662612a4eaeab391f89bcc58f010b3a90dd8 |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.10
git worktree add -d .worktree/backport-6971-to-2.10 origin/2.10
cd .worktree/backport-6971-to-2.10
git switch --create backport-6971-to-2.10
git cherry-pick -x ca5b662612a4eaeab391f89bcc58f010b3a90dd8 |
) * fix: fix reporting used memory as unsigned long long * change the bytes collected type to double again after change by other PR (cherry picked from commit ca5b662)
|
Successfully created backport PR for |
) * fix: fix reporting used memory as unsigned long long * change the bytes collected type to double again after change by other PR (cherry picked from commit ca5b662)
|
Successfully created backport PR for |
) * fix: fix reporting used memory as unsigned long long * change the bytes collected type to double again after change by other PR (cherry picked from commit ca5b662)
) * fix: fix reporting used memory as unsigned long long * change the bytes collected type to double again after change by other PR (cherry picked from commit ca5b662)
) * fix: fix reporting used memory as unsigned long long * change the bytes collected type to double again after change by other PR (cherry picked from commit ca5b662)
…ng (#7276) * [RED-169833] fix: fix reporting used memory as unsigned long long (#6971) * fix: fix reporting used memory as unsigned long long * change the bytes collected type to double again after change by other PR (cherry picked from commit ca5b662) * fix: correct stats for gc collected bytes --------- Co-authored-by: Joan Fontanals <[email protected]>
…ng (#7275) * [RED-169833] fix: fix reporting used memory as unsigned long long (#6971) * fix: fix reporting used memory as unsigned long long * change the bytes collected type to double again after change by other PR (cherry picked from commit ca5b662) * fix: correct stats for gc collected bytes --------- Co-authored-by: Joan Fontanals <[email protected]>
2 participants
Describe the changes in the pull request
We are reporting max memory from the module as
doublewhich could lead to errors, while we have a size_t value, we should report as ULongLongNote
Switch INFO metrics from double to unsigned long long for memory, GC, and error counters to avoid precision issues.
used_memory_indexes,smallest_memory_index,largest_memory_index,used_memory_vector_indexnow useRedisModule_InfoAddFieldULongLong; human-readable fields unchanged.gc_total_cycles,gc_total_ms_runswitched to unsigned long long (keptgc_bytes_collectedas double).errors_indexing_failures,errors_for_index_with_max_failures,OOM_indexing_failures_indexes_countswitched to unsigned long long.Written by Cursor Bugbot for commit 701ceda. This will update automatically on new commits. Configure here.