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.

Sign up for GitHub

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

Jump to bottom

Add support for datadog distributions metric #8179

Merged
merged 11 commits into from
Feb 16, 2021
Merged

Add support for datadog distributions metric #8179

merged 11 commits into from
Feb 16, 2021

Conversation

ppsreejith
Copy link
Contributor

@ppsreejith ppsreejith commented Sep 25, 2020 •
edited by sjwang90
Loading

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Thanks for the awesome work you folks do :)

This PR is my attempt to add support for the distributions metric (alternate link) to maintain compatibility with Datadog metrics. This would allow us to aggregate metrics on the server rather than on the agent (i.e. aggregate on Influx rather than on Telegraf).

I've added unit tests and tested this on my local Influx and Telegraf setup. Let me know of all the changes required 👍

Closes #8134

@ppsreejith ppsreejith changed the title Add support for datadog distributions Add support for datadog distributions metric Sep 25, 2020
@sjwang90
Copy link
Contributor

Awesome! Thanks @ppsreejith. We currently have a backlog of PR reviews for our next 1.16 release which we want out soon. We'll get this reviewed shortly after.

@gerilya
Copy link

gerilya commented Nov 16, 2020

Awesome! Thanks @ppsreejith. We currently have a backlog of PR reviews for our next 1.16 release which we want out soon. We'll get this reviewed shortly after.

@sjwang90 as v1.16 went out approx one month ago, I am wondering whether you have an ETA for merging this nice feature into the master branch. Thank you

srebhan
srebhan reviewed Nov 20, 2020
View reviewed changes
plugins/inputs/statsd/statsd.go Outdated
Comment on lines 755 to 776
fields := make(map[string]interface{})
fields[m.field] = m.floatvalue
cached := cacheddistributions{
name: m.name,
fields: fields,
tags: m.tags,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are always setting just one field, why do you use a map for this? You could also store the name and value instead of allocating over and over.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I updated the cacheddistributions interface and replaced field with value. The repeated allocation now happens in the Gather method where the accumulator's addFields requires a map argument.

srebhan
srebhan requested changes Nov 20, 2020
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have some comments in the code, nothing big. Please also add the new option to the sampleConfig variable!

@srebhan srebhan self-assigned this Nov 20, 2020
@srebhan
Copy link
Member

srebhan commented Dec 3, 2020

@ppsreejith any news?

@sjwang90 sjwang90 added the wip label Dec 3, 2020
@sjwang90 sjwang90 added this to the Planned milestone Dec 9, 2020
@ppsreejith
Copy link
Contributor Author

@srebhan Sorry for the (really) long pause (Life's been hectic 😅). I've some time now so I'll pick this up this week 👍

@ppsreejith ppsreejith force-pushed the feature/datadog-distributions branch from 6ad36e0 to 0b298bb Compare January 6, 2021 09:08
@ppsreejith
Copy link
Contributor Author

@srebhan I've addressed the above comments 👍 Let me know if there are more changes.

Please also add the new option to the sampleConfig variable!

Do you mean the sampleConfig defined in the plugins/inputs/statsd/statsd.go file? I'm not sure what changes would be required here.

@ppsreejith
Copy link
Contributor Author

Also, small side note: I set up a playground to make it easier to test out Telegraf changes end-to-end (In case this makes it easier for others).

@srebhan
Copy link
Member

srebhan commented Jan 18, 2021

@ppsreejith to be honest I can't remember what I meant with

Please also add the new option to the sampleConfig variable!

:-) However, as you add new fields it might be good to give the user a chance to enable or disable the new field(s). We want to keep the output series stable between versions by default. Is there a way to enable/disable the distribution fields? If not, could you please add a flag to the config where those fields are disabled by default!?

@ppsreejith
Copy link
Contributor Author

@srebhan Got it. I'll add this 👍

@ppsreejith ppsreejith force-pushed the feature/datadog-distributions branch from 81ca641 to 06dff47 Compare January 27, 2021 06:08
@ppsreejith
Copy link
Contributor Author

@srebhan I've added a new config flag datadog_distributions to enable parsing the distributions metric.

The distributions metric will only be parsed if both the datadog_extensions config and the datadog_distributions config are enabled. My reasoning is that since the distributions metric type is a datadog extension, we should only enable parsing if the datadog_extensions config is enabled. Furthermore, to keep the option of enabling/disabling this feature, I've added the new datadog_distributions config.

I've documented this config in the sampleConfig variable as well as the Readme file (with links to Datadog's documentation). I've also extended the tests to cover this config enabled parsing. Let me know if this looks good or if there are other changes required 👍

srebhan
srebhan approved these changes Jan 27, 2021
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your effort! Looks good to me!

@srebhan srebhan added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. and removed wip labels Jan 27, 2021
@sjwang90 sjwang90 removed this from the Planned milestone Jan 29, 2021
@Hipska Hipska added the plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins label Jan 29, 2021
@sspaink sspaink merged commit f888136 into influxdata:master Feb 16, 2021
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
@ppsreejith
Add support for datadog distributions metric (influxdata#8179)
746b29d 
* Add support for datadog distributions in statsd

* Parse metric distribution correctly

* Add tests to check distributions are parsed correctly

* Update Statsd plugin Readme with details about Distributions metric

* Refactor metric distribution initialization code

* Update distribution metric interface to replace fields with value

* Refactor statsd distribution metric test code

* Fix go formatting errors

* Add tests to parse only when DataDog Distributions config is enabled

* Add config to enable parsing DataDog Statsd Distributions

* Document use of datadog_distributions config in Readme
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srebhan srebhan srebhan approved these changes

Assignees

@srebhan srebhan

Labels
area/statsd feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[inputs/statsd] Add support for Distribution metrics for compatibility with DataDog (DogStatsD)
6 participants
@ppsreejith @sjwang90 @gerilya @srebhan @Hipska @sspaink