Skip to content
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

Merged
merged 26 commits into from
Aug 13, 2019
Merged

Add statsd module #13109

merged 26 commits into from
Aug 13, 2019

Conversation

roncohen
Copy link
Contributor

@roncohen roncohen commented Jul 30, 2019

This adds a statsd endpoint.

Does not support sample rate yet.

Closes #10426

@roncohen roncohen added Team:Integrations Label for the Integrations team Metricbeat Metricbeat labels Jul 30, 2019
@roncohen roncohen changed the title Add statsd endpoint Add statsd module Jul 30, 2019
Copy link
Contributor

@exekias exekias left a 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
Copy link
Contributor

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)

Copy link
Contributor Author

@roncohen roncohen Jul 31, 2019

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?

Copy link
Contributor

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

Copy link
Contributor Author

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' ?

Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

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

@exekias exekias added the module label Jul 31, 2019
@roncohen roncohen marked this pull request as ready for review July 31, 2019 12:05
@roncohen roncohen requested review from a team as code owners July 31, 2019 12:05
@exekias exekias added the in progress Pull request is currently in progress. label Jul 31, 2019
@roncohen
Copy link
Contributor Author

unrelated failure
jenkins, retest this please

Statsd module
release: beta
fields:
- name: statsd.*
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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

@roncohen
Copy link
Contributor Author

roncohen commented Aug 1, 2019

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.

@roncohen
Copy link
Contributor Author

roncohen commented Aug 1, 2019

mapping isn't working at the moment, will fix that tomorrow

Copy link
Member

@jsoriano jsoriano left a 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},
Copy link
Member

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.

@roncohen
Copy link
Contributor Author

great comments, thank @jsoriano !

@roncohen roncohen merged commit dbeec8a into elastic:master Aug 13, 2019
@jsoriano jsoriano added test-plan Add this PR to be manual test plan v7.4.0 labels Aug 13, 2019
@roncohen roncohen removed the in progress Pull request is currently in progress. label Aug 13, 2019
roncohen pushed a commit that referenced this pull request Aug 15, 2019
gauges can be inc/dec/set while counters are just inc and get reset after each report

Follow up on #13109
@roncohen roncohen deleted the statsd branch September 2, 2019 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Metricbeat Metricbeat module Team:Integrations Label for the Integrations team test-plan Add this PR to be manual test plan v7.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metricbeat: statsd module
5 participants