decode: remove summary quantile limit. #168
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
While running with this configuration I get a two Invalid reads in valgrind: And here is the valgrind log: It does not happen for each record, it happens only twice. Any ideas @leonardo-albertovich or @edsiper? |
|
@tarruda can you take a look at this from the Prometheus decoding perspective ? @leonardo-albertovich can you check the logic for msgpack encoding/decoding here? |
tarruda
reviewed
Nov 4, 2022
| @@ -130,7 +130,7 @@ double cmt_summary_quantile_get_value(struct cmt_metric *metric, int quantile_id | |||
| { | |||
| uint64_t val; | |||
|
|
|||
| if (quantile_id < 0 || quantile_id > 5) { | |||
| if (quantile_id < 0 /*|| quantile_id > metric->sum_quantiles_count*/) { | |||
There was a problem hiding this comment.
Is it possible the invalid read is because of this commented check? Even though valgrind reports this as happening on cmt_summary_set_default, it is possible the compiler inlined cmt_summary_quantile_get_value/ cmt_summary_quantile_set_value
There was a problem hiding this comment.
it is entirely possible. That check was commented while I worked out the best way to check the limit. The limit before was just hardcoded.
|
fixed the invalid read (plus another invalid parsing bug) in #170 |
edsiper
pushed a commit
that referenced
this pull request
Nov 4, 2022
* decode: prometheus: Add failing test for summary issue Signed-off-by: Thiago Padilha <[email protected]> * decode: prometheus: Fix pr 168 summary issue Signed-off-by: Thiago Padilha <[email protected]> Signed-off-by: Thiago Padilha <[email protected]>
Signed-off-by: Phillip Whelan <[email protected]>
|
Here is a valgrind log of this patch running with fluent-bit 2.0.4: |
pwhelan
force-pushed
the
fix-quantile-limit
branch
from
f061751 to
c74ddc7
Compare
|
@edsiper this is ready for merging and/or review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove the self-imposed limit of 5 quantile summaries, re #167. This causes a bug when decoding prometheus metrics.