-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add statsd module #13109
Add statsd module #13109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a first pass, this is looking great! it will probably need a make update
to make CI happy
for mk, mv := range v { | ||
metric[mk] = mv | ||
} | ||
fields[k] = metric |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, this is how mv will look: https://github.com/rcrowley/go-metrics/blob/9beb055b7962d16947a14e1cd718098a2431e20e/registry.go#L127
I think it should not cause issues, as keys don't collide. I'm wondering if mapping will break for the ones containing dots, as the resulting field name will be: statsd.my_histogram.99.9%
(notice the final dot)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. Should i just replace with dots with underlines in the value or what's the standard way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM, that's what we normally do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is '%' a problem? replace with 'pct' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't be a problem for Elasticsearch, I wonder if some parts of Kibana could be affected. Adding pct may be misleading too, as someone could think this value is a percentage.
How about something like p95
, p99
, p99_9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i went for 95p etc for now as it was simple to do. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds ok, I see the resulting metrics are like statsd.metric4_95p
. We normally would do statsd.metric4.95p
, for instance, for AWS Cloudwatch metrics, where the statistic is after the dot. That said... I don't have an strong opinion here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated 👍 thanks for the input
unrelated failure |
Statsd module | ||
release: beta | ||
fields: | ||
- name: statsd.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using statsd.metrics.*
for the metrics? In case we want to add additional statsd metadata as the tags at some moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see we are adding the tags to the labels
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the mappings. Would be great to get some input
I've updated the mapping to use float instead of double. This is from the assumption that it will save a bit of disk space and I don't expect the precision loss to matter. |
mapping isn't working at the moment, will fix that tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job here, this is going to be a great addition to metricbeat 👍
|
||
events[idx] = &mb.Event{ | ||
MetricSetFields: sanitizedMetrics, | ||
RootFields: common.MapStr{"labels": mapstrTags}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always have the concern of using labels
ECS field for different kind of labels. But let's keep it, we can always filter by service.type
if we want different querying depending on the service.
great comments, thank @jsoriano ! |
gauges can be inc/dec/set while counters are just inc and get reset after each report Follow up on #13109
This adds a statsd endpoint.
Does not support sample rate yet.
Closes #10426