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

enhancement: Add DataDog's distribution metric #2913

Merged
merged 22 commits into from Aug 5, 2020

Conversation

ktff
Copy link
Contributor

@ktff ktff commented Jun 28, 2020

Ref. #2603

PR expands the following:

  • Our metric scheme with summary metric by expanding MetricValue::Distribution to have StatisticKind.
  • statsd source to accept d.
  • statsd sink to emit d.

Other sinks treat the new metric as histogram.

Documentation

Minimal amount has been changed.

We could update documentation for metric sinks:

  • aws_cloudwatch_metrics
  • datadog_metrics
  • prometheus
  • influxdb_metrics

but I'm unsure if we would like to do it now in this state.

Naming

Instead of name distribution, summary was chosen for clarity in context of our metric scheme, and MetricValue::Distribution was not renamed so to avoid breaking change.

Todo

  • Use some other naming scheme?

  • Is changing proto a breaking change? (EDIT: in this case not)

  • Documentation

    • Update website documentation.

    • Set timestamp to null in website documentation for statsd source ? (EDIT: They show the end result)

    • Recheck existing docmentation.

@ktff ktff requested a review from lukesteensen June 28, 2020 17:51
@ktff ktff self-assigned this Jun 28, 2020
Copy link
Member

@lukesteensen lukesteensen left a comment

Choose a reason for hiding this comment

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

I find it confusing that a Distribution can either be a Distribution or a Histogram when we also have a top level AggregatedHistogram. Could you explain why we've modeled it this way? I didn't see anything obvious in the linked Datadog documentation.

@ktff
Copy link
Contributor Author

ktff commented Jul 3, 2020

@lukesteensen It has more to do with our code than DataDogs. The original MetricValue::Distribution could be split into two parts:

  1. Collection of sample values
  2. Aggregating those samples

In the original version, the 2. part was calculating histogram. Since this new DataDog distribution also has those same steps with the 2. one being different, it made sense to explicitly split MetricValue::Distribution into those two steps so that we could reuse 1. step and have a bit simpler model since we now expose through type that there are two steps in this.

AggregatedHistorgram is just that, samples that have passed through 2. step which we only do in some of the sinks. If we would aggregate the samples in the sources we would do 2.step for all of the sinks which is not clear that we would want and would also be a breaking change.

Regarding naming scheme, I agree, it is confusing, the top level Distribution is now more akin to sample collection, so Samples could be a more appropriate name.

@ktff
Copy link
Contributor Author

ktff commented Jul 7, 2020

@lukesteensen Distribution has been renamed to better reflect what it is.

@ktff ktff requested a review from lukesteensen July 7, 2020 18:49
@ktff ktff requested a review from lukesteensen July 9, 2020 18:17
@ktff ktff mentioned this pull request Jul 10, 2020
@ktff ktff force-pushed the ktff/statsd_distribution_metric branch from 4d58c78 to 5311c4c Compare July 10, 2020 11:32
@ktff ktff force-pushed the ktff/statsd_distribution_metric branch from 5311c4c to f16f9fc Compare July 14, 2020 10:14
@ktff
Copy link
Contributor Author

ktff commented Jul 17, 2020

cc @lukesteensen

@lukesteensen
Copy link
Member

Moving forward, I think the best thing to do here is to accept the d statsd type in our statsd source and collect them into the histogram data type. Anything more complex than that is going to require a bit of research and experimentation.

@ktff
Copy link
Contributor Author

ktff commented Jul 21, 2020

accept the d statsd type in our statsd source

@lukesteensen what about extending this to statsd sink so that it can output d type, and every other sink can deal with it as a histogram? With it we can, accept d, pass it through, and we would have the backbone to approach this later, incrementally if needed.

@lukesteensen
Copy link
Member

@ktff That sounds like a good plan to me!

@ktff ktff force-pushed the ktff/statsd_distribution_metric branch 2 times, most recently from 38cdbb4 to c67f68c Compare July 23, 2020 14:53
@ktff ktff requested a review from binarylogic as a code owner July 23, 2020 16:58
@ktff
Copy link
Contributor Author

ktff commented Jul 23, 2020

@binarylogic
Regarding timestamp in documentation of statsd source, examples show a concrete timestamp, while we are later mentioning that they are null value, which is also true in code. Going by this, examples should show null?

@ktff ktff force-pushed the ktff/statsd_distribution_metric branch from 62e1def to 5a7f220 Compare July 30, 2020 13:23
@ktff
Copy link
Contributor Author

ktff commented Jul 30, 2020

@binarylogic @lukesteensen could you review this?

This turned out more complex than anticipated so after merging this I'll update #2603 with a proper plan.

I'm also unsure how to fix the netlify checks.

@ktff ktff force-pushed the ktff/statsd_distribution_metric branch from 5a7f220 to 855e88f Compare July 31, 2020 13:40
Copy link
Contributor

@binarylogic binarylogic left a comment

Choose a reason for hiding this comment

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

Docs look good

Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
ktff added 16 commits August 5, 2020 09:09
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
@ktff ktff force-pushed the ktff/statsd_distribution_metric branch from 855e88f to d866f67 Compare August 5, 2020 07:10
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
@ktff ktff merged commit 547c8ad into master Aug 5, 2020
@ktff ktff deleted the ktff/statsd_distribution_metric branch August 5, 2020 13:24
@binarylogic binarylogic added domain: data model Anything related to Vector's internal data model domain: metrics Anything related to Vector's metrics events and removed event type: metric labels Aug 6, 2020
mengesb pushed a commit to jacobbraaten/vector that referenced this pull request Dec 9, 2020
* Add StatisticKind

Signed-off-by: ktf <krunotf@gmail.com>

* Update tests

Signed-off-by: ktf <krunotf@gmail.com>

* Update aws_cloudwatch_metrics sink

Signed-off-by: ktf <krunotf@gmail.com>

* Update lua transform

Signed-off-by: ktf <krunotf@gmail.com>

* Fix build

Signed-off-by: ktf <krunotf@gmail.com>

* Fix test

Signed-off-by: ktf <krunotf@gmail.com>

* Add test

Signed-off-by: ktf <krunotf@gmail.com>

* Update remaining usages of Distribution

Signed-off-by: ktf <krunotf@gmail.com>

* Update tests

Signed-off-by: ktf <krunotf@gmail.com>

* Rename to Samples

Signed-off-by: ktf <krunotf@gmail.com>

* Update test

Signed-off-by: ktf <krunotf@gmail.com>

* Rename stray Distribution

Signed-off-by: ktf <krunotf@gmail.com>

* Update statsd sink

Signed-off-by: ktf <krunotf@gmail.com>

* Update cherry picked

Signed-off-by: ktf <krunotf@gmail.com>

* Simpler naming

Signed-off-by: ktf <krunotf@gmail.com>

* Update docs

Signed-off-by: ktf <krunotf@gmail.com>

* Update

Signed-off-by: ktf <krunotf@gmail.com>

* Remove trailling space

Signed-off-by: ktf <krunotf@gmail.com>

* Remove unused

Signed-off-by: ktf <krunotf@gmail.com>

* Add new line

Signed-off-by: ktf <krunotf@gmail.com>

* Bump

Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
Signed-off-by: Brian Menges <brian.menges@anaplan.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: data model Anything related to Vector's internal data model domain: metrics Anything related to Vector's metrics events
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants