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

[OpenMetricsV2] Add an option to send sum and count information when using distribution metrics #9301

Merged
merged 3 commits into from
May 11, 2021

Conversation

@olivielpeau
Copy link
Member

The count aggregation on the distribution metric in-app should be fully accurate AFAIK, is there any reason to send it separately?

I understand the sum aggregation on the distribution metric would not be fully accurate though, so it likely makes sense to send it separately as a DD count.

Side note: this will cause merge conflicts with #9276 cc @gh123man

@ofek
Copy link
Contributor Author

ofek commented May 6, 2021

@olivielpeau If we send one, we should send both imo to avoid confusion. The requests also desire both. WDYT?

@olivielpeau
Copy link
Member

The main downside I can think of is that for openmetrics integrations that send metrics counted as custom metrics, this would send one more custom metric (count) per distribution bucket even though it's not strictly necessary.

@ofek
Copy link
Contributor Author

ofek commented May 7, 2021

The customers in the linked issue consider both to be useful actually, so I think this is worth it

@ofek
Copy link
Contributor Author

ofek commented May 7, 2021

Added a flag

@ofek ofek requested a review from mfpierre May 7, 2021 23:28
@ofek ofek changed the title [OpenMetricsV2] Send sum and count information when using distribution metrics [OpenMetricsV2] Add an option to send sum and count information when using distribution metrics May 7, 2021
olivielpeau
olivielpeau previously approved these changes May 9, 2021
Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

Haven't reviewed the code in details, but behavior LGTM :)

(the flag should probably be documented?)

kayayarai
kayayarai previously approved these changes May 10, 2021
@ofek
Copy link
Contributor Author

ofek commented May 11, 2021

Rebased to fix a bunch of ci jobs

Copy link
Contributor

@mfpierre mfpierre left a comment

Choose a reason for hiding this comment

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

LGTM, I agree that it shouldn't be the default behavior as the count is duplicate info from what we already have in the distribution metric, but this could be useful for people who really want the accurate sum of values (although percentiles on the distribution metrics could be more useful in an alerting context)

@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

Merging #9301 (d382a2f) into master (90532d9) will increase coverage by 0.00%.
The diff coverage is 88.88%.

Flag Coverage Δ
activemq_xml 82.31% <ø> (ø)
amazon_msk 89.53% <ø> (ø)
ambari 86.98% <ø> (ø)
apache 94.90% <ø> (ø)
aspdotnet 93.87% <ø> (ø)
azure_iot_edge 82.01% <ø> (ø)
btrfs 82.91% <ø> (ø)
cacti 84.01% <ø> (ø)
cilium 85.84% <ø> (+1.88%) ⬆️
cisco_aci 95.88% <ø> (ø)
cloud_foundry_api 95.98% <ø> (+0.12%) ⬆️
couchbase 81.45% <ø> (ø)
eks_fargate 94.05% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ts/openmetrics/test_transformers/test_histogram.py 100.00% <ø> (ø)
...se/checks/openmetrics/v2/transformers/histogram.py 94.73% <88.00%> (ø)
...dog_checks/base/checks/openmetrics/v2/transform.py 85.32% <100.00%> (ø)
zk/tests/conftest.py 92.22% <0.00%> (-2.23%) ⬇️
...adog_checks/cloud_foundry_api/cloud_foundry_api.py 93.70% <0.00%> (+0.34%) ⬆️
spark/datadog_checks/spark/spark.py 86.61% <0.00%> (+0.52%) ⬆️
cilium/tests/conftest.py 78.84% <0.00%> (+3.84%) ⬆️

@ofek ofek merged commit 54232b6 into master May 11, 2021
@ofek ofek deleted the ofek/o branch May 11, 2021 15:59
@mhamrah
Copy link

mhamrah commented Sep 30, 2021

We're sending OpenTelemetry Histograms via a prometheus exporter to Datadog via the agent and the openmetrics check. We know the agent sends these to datadog as distributions, and are seeing very odd counts for these distributions. Does this issue sound familiar, is this PR meant to address this issue on histograms -> distributions -> accurate counts?

TY. For ref, datadog support tickets 565850 and 567929 are related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants