Add support for datadog distributions metric by ppsreejith · Pull Request #8179 · influxdata/telegraf

ppsreejith · 2020-09-25T09:18:03Z

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

sjwang90 · 2020-09-25T16:05:13Z

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 · 2020-11-16T07:16:17Z

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

plugins/inputs/statsd/statsd.go

srebhan · 2020-11-20T17:31:38Z

plugins/inputs/statsd/statsd.go

+		fields := make(map[string]interface{})
+		fields[m.field] = m.floatvalue
+		cached := cacheddistributions{
+			name:   m.name,
+			fields: fields,
+			tags:   m.tags,
+		}


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.

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

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

srebhan · 2020-12-03T14:45:54Z

@ppsreejith any news?

ppsreejith · 2020-12-29T17:10:15Z

@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 · 2021-01-12T06:45:18Z

@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 · 2021-01-12T07:08:30Z

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 · 2021-01-18T15:34:24Z

@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 · 2021-01-19T01:03:42Z

@srebhan Got it. I'll add this 👍

ppsreejith · 2021-01-27T07:17:59Z

@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

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

* 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

ppsreejith changed the title ~~Add support for datadog distributions~~ Add support for datadog distributions metric Sep 25, 2020

sjwang90 added the area/statsd label Sep 25, 2020

srebhan reviewed Nov 20, 2020

View reviewed changes

plugins/inputs/statsd/statsd.go Outdated Show resolved Hide resolved

srebhan reviewed Nov 20, 2020

View reviewed changes

plugins/inputs/statsd/statsd.go Outdated Show resolved Hide resolved

srebhan reviewed Nov 20, 2020

View reviewed changes

srebhan requested changes Nov 20, 2020

View reviewed changes

srebhan self-assigned this Nov 20, 2020

sjwang90 added the wip label Dec 3, 2020

sjwang90 added this to the Planned milestone Dec 9, 2020

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

ppsreejith added 8 commits January 27, 2021 11:38

Add support for datadog distributions in statsd

5aeecc0

Parse metric distribution correctly

43cff69

Add tests to check distributions are parsed correctly

3f1839c

Update Statsd plugin Readme with details about Distributions metric

cb04798

Refactor metric distribution initialization code

f8d5311

Update distribution metric interface to replace fields with value

100181d

Refactor statsd distribution metric test code

fe5397d

Fix go formatting errors

06dff47

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

ppsreejith added 3 commits January 27, 2021 12:23

Add tests to parse only when DataDog Distributions config is enabled

5ce077a

Add config to enable parsing DataDog Statsd Distributions

19a066f

Document use of datadog_distributions config in Readme

e8e65b2

srebhan approved these changes Jan 27, 2021

View reviewed changes

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 removed this from the Planned milestone Jan 29, 2021

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 merged commit f888136 into influxdata:master Feb 16, 2021

Conversation

ppsreejith commented Sep 25, 2020 • edited by sjwang90 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Required for all PRs:

Uh oh!

sjwang90 commented Sep 25, 2020

Uh oh!

gerilya commented Nov 16, 2020

Uh oh!

Uh oh!

Uh oh!

srebhan Nov 20, 2020

Choose a reason for hiding this comment

Uh oh!

ppsreejith Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

srebhan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srebhan commented Dec 3, 2020

Uh oh!

ppsreejith commented Dec 29, 2020

Uh oh!

ppsreejith commented Jan 12, 2021

Uh oh!

ppsreejith commented Jan 12, 2021

Uh oh!

srebhan commented Jan 18, 2021

Uh oh!

ppsreejith commented Jan 19, 2021

Uh oh!

ppsreejith commented Jan 27, 2021

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ppsreejith commented Sep 25, 2020 •

edited by sjwang90

Loading

srebhan left a comment •

edited

Loading