feat(inputs.unbound): Collect histogram statistics #16452
Conversation
|
Thanks so much for the pull request! |
srebhan
changed the title
srebhan
changed the title
telegraf-tiger
bot
added
feat
plugins/inputs/unbound/unbound.go
Outdated
| } else if strings.HasPrefix(stat, "histogram") { | ||
| statTokens := strings.Split(stat, ".") | ||
| if len(statTokens) > 1 { | ||
| lbound, err := strconv.ParseFloat(strings.Join(statTokens[1:3], "."), 64) | ||
| if err != nil { | ||
| acc.AddError(fmt.Errorf("expected a numeric value for the histogram bucket lower bound: %s", strings.Join(statTokens[1:3], "."))) | ||
| continue | ||
| } | ||
| field := fmt.Sprintf("%s_%f", statTokens[0], lbound) | ||
| fields[field] = fieldValue | ||
| } | ||
| } else { |
There was a problem hiding this comment.
How about
| } else if strings.HasPrefix(stat, "histogram") { | |
| statTokens := strings.Split(stat, ".") | |
| if len(statTokens) > 1 { | |
| lbound, err := strconv.ParseFloat(strings.Join(statTokens[1:3], "."), 64) | |
| if err != nil { | |
| acc.AddError(fmt.Errorf("expected a numeric value for the histogram bucket lower bound: %s", strings.Join(statTokens[1:3], "."))) | |
| continue | |
| } | |
| field := fmt.Sprintf("%s_%f", statTokens[0], lbound) | |
| fields[field] = fieldValue | |
| } | |
| } else { | |
| } else if suffix, found := strings.CutPrefix(x, "histogram."); found { | |
| statTokens := strings.Split(stat, ".") | |
| fields["histogram_"+suffix] = fieldValue | |
| } else { |
There was a problem hiding this comment.
This was my first stab at Go, so apologies for style and efficiency issues.
Your suggestion is indeed simpler but - and this is a personal opinion - I think field names become unnecessarily large, and harder to parse and post-process (histogram_000064.000000.to.000128.000000 vs histogram_64.000000). The upper bounds of the histogram are redundant, as they'll always be the lower bound of the next bucket. So the longer field names bring no additional information, but are harder to analyze. But again, this is an opinion, I can understand if you want to keep things closer to the original data. I'll change the PR if needed.
(I also found two issues with your suggested code: x on line 96 should be stat, and line 97 can be dropped, as it raises an error about statTokens being defined but not uses, but that's a detail)
The option for collecting these stats sounds like a good idea. I'll work on that.
There was a problem hiding this comment.
I pushed two additional commits adding an option for collecting histogram statistics, and documenting it. Let me know how you stand on the field names issue. I'm also considering dropping this PR, which is getting a bit messy, and make a new cleaner one, once it is all settled.
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
Summary
The histogram data on unbound response times is more informative than the (already exported) mean and median values.
The bounds of the histogram's buckets are hardcoded (see
util/timehist.con the unbound source) so it is safe to assume they will remain constant. Therefore, it is sufficient to only export the lower bound of each bucket, in the field name.Checklist
Related issues
resolves #16451