tail plugin, latency: Fix regressions after #2535 #2611
Merged
Conversation
This file contains hidden or 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
octo
reviewed
Dec 12, 2017
src/utils_tail_match.c
Outdated
| @@ -118,10 +118,10 @@ static int latency_submit_match(cu_match_t *match, void *user_data) { | |||
| sstrncpy(vl.type, data->type, sizeof(vl.type)); | |||
| for (size_t i = 0; i < data->latency_config.percentile_num; i++) { | |||
| if (strlen(data->type_instance) != 0) | |||
| snprintf(vl.type_instance, sizeof(vl.type_instance), "%.117s-%.2f", | |||
| snprintf(vl.type_instance, sizeof(vl.type_instance), "%.50s-%.4g", | |||
There was a problem hiding this comment.
This format was originally "%s-%.0f". Can we go back to using %.0f again?
There was a problem hiding this comment.
As stated before, that is done to allow to report percentiles with fractional values.
# perl -e 'use POSIX; printf("%.0f\n", 99.5);'
100
# perl -e 'use POSIX; printf("%.4g\n", 99.5);'
99.5
# perl -e 'use POSIX; printf("%.4g\n", 99);'
99
Configuration allows to set fractional percentile values, so I think we need to allow report them correctly.
If you want, we can split this change - set %.0f in 5.8 and %.4g in master. Or we can just set to %.0f. What would be preferred?
There was a problem hiding this comment.
The docs don't say that fractional percentiles are forbidden, so it makes sense to support them. Put the %g variant into 5.8.
Maybe we can increase the precision to %.5g though – somebody is going to want to track five nines … ;)
- Remove two fixed decimal places from type instance of percentiles - Recover accuracy of bounds in type instance of buckets + Allow (compared to state before collectd#2535) to report percentiles with fractional values * Changed maximum length of reported type and type instance to 50 chars References: collectd#2535 Closes: collectd#2587
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.
As reported in #2587, the f46d76e causes regression.
Before:
After:
Also, the difference in format of percentile appeared after f46d76e : the '%.0f' was in use when no type instance is configured, and '%.2f' - when type instance is set.
This was fixed by setting format to '%.4g' for all cases.
I think it's enough to use '%.4g' to output percentile values which are in range from 0 to 100.
Before:
After:
This was fixed by reverting format back to '%g' as it was before f46d76e.
Summary:
References: #2535, #2587
Have anybody any objections against merging this?
CC @taraschornyi, @mfournier, @PhantomPhreak