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

feat: do not send any histogram or summary metric #3818

Merged
merged 6 commits into from
Aug 2, 2024

Conversation

sumo-drosiek
Copy link
Contributor

@sumo-drosiek sumo-drosiek commented Aug 1, 2024

Checklist

Histogram is a lot of data points which are not used on Sumo Logic side by default. In case we will need to send some of them, we can figure out additional flag which will be an exception from that rule, but for now it sounds reasonable to stop ingesting buckets at all

The same is about summary and quantiles

  • Changelog updated or skip changelog label added
  • Documentation updated
  • Template tests added for new features
  • Integration tests added or modified for major features

@sumo-drosiek sumo-drosiek requested a review from a team as a code owner August 1, 2024 10:18
@kasia-kujawa
Copy link
Contributor

Please remember to add changelog entry.

@sumo-drosiek sumo-drosiek force-pushed the drosiek-drop-all-histograms branch from 29cab66 to d125e88 Compare August 1, 2024 12:59
@sumo-drosiek
Copy link
Contributor Author

sumo-drosiek commented Aug 1, 2024

Not quite sure about that, but we may probably also want to do the same for summary metrics
https://prometheus.io/docs/concepts/metric_types/#summary

Dominik Rosiek added 2 commits August 1, 2024 15:34
Signed-off-by: Dominik Rosiek <[email protected]>
@sumo-drosiek sumo-drosiek force-pushed the drosiek-drop-all-histograms branch from ac6afb3 to 7ef371f Compare August 1, 2024 13:45
@sumo-drosiek sumo-drosiek changed the title chore: do not send any histogram data feat: do not send any histogram or summary metric Aug 1, 2024
Signed-off-by: Dominik Rosiek <[email protected]>
@sumo-drosiek sumo-drosiek force-pushed the drosiek-drop-all-histograms branch from 9932698 to f3f89b4 Compare August 1, 2024 14:37
@@ -16,7 +16,7 @@ filter/drop_unnecessary_metrics:
- resource.attributes["job"] != "pod-annotations" and IsMatch(name, "scrape_.*")
{{- if .Values.sumologic.metrics.dropHistogramBuckets }}
# drop histograms we've extracted sums and counts from, but don't want the full thing
- IsMatch(name, "^(apiserver_request_duration_seconds|coredns_dns_request_duration_seconds|kubelet_runtime_operations_duration_seconds)$")
- type == METRIC_DATA_TYPE_HISTOGRAM or type == METRIC_DATA_TYPE_EXPONENTIAL_HISTOGRAM or type == METRIC_DATA_TYPE_SUMMARY or IsMatch(name, ".*_bucket")
Copy link
Contributor

@rnishtala-sumo rnishtala-sumo Aug 1, 2024

Choose a reason for hiding this comment

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

maybe we should allow one histogram metric to demonstrate, how a customer could make exceptions. Something like - not (IsMatch(name, "^(apiserver_request_duration_seconds)$")) and type == METRIC_DATA_TYPE_HISTOGRAM or type == METRIC_DATA_TYPE_EXPONENTIAL_HISTOGRAM or type == METRIC_DATA_TYPE_SUMMARY or IsMatch(name, ".*_bucket"

Copy link
Contributor Author

@sumo-drosiek sumo-drosiek Aug 2, 2024

Choose a reason for hiding this comment

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

I would expose new configuration option, and conditionally adding not isMatch at the end of the query

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 wouldn't play with merge if we can do it easily by the configuration option

@@ -16,7 +16,7 @@ filter/drop_unnecessary_metrics:
- resource.attributes["job"] != "pod-annotations" and IsMatch(name, "scrape_.*")
Copy link
Contributor

@rnishtala-sumo rnishtala-sumo Aug 2, 2024

Choose a reason for hiding this comment

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

should we also drop metrics coming in from pod annotations by default as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's breaking change

@sumo-drosiek sumo-drosiek merged commit b6c474c into main Aug 2, 2024
55 checks passed
@sumo-drosiek sumo-drosiek deleted the drosiek-drop-all-histograms branch August 2, 2024 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants