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

Do not pass histogram metrics to the fluentd #595

Merged
merged 16 commits into from
May 4, 2020
Merged

Do not pass histogram metrics to the fluentd #595

merged 16 commits into from
May 4, 2020

Conversation

sumo-drosiek
Copy link
Contributor

Description

Drop histogram metrics (with _bucket suffix)

Testing performed
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

@sumo-drosiek
Copy link
Contributor Author

For 5min test I reduced amount of data points:
master: 434 382
without apiserver and kublet: 129227
no _bucket metrics at all: 124149

I'm not sure if we want to drop all metrics (e.g. we are using prometheus_remote_storage_sent_batch_duration_seconds_bucket for investigation purposes)

@sumo-drosiek sumo-drosiek changed the title Drosiek drop histogram metrics Do not pass histogram metrics to the fluentd Apr 27, 2020
Dominik Rosiek and others added 2 commits April 28, 2020 09:36
Revert "Drop all bucket metrics"
40447dd 
Revert this as we are using some of the  metrics for investigation
purposes and they are small overhead

This reverts commit 0a94407.
Generate new overrides yaml/libsonnet file(s).
06a7553
@sumo-drosiek sumo-drosiek added this to the v1.0 milestone Apr 28, 2020
perk-sumo
perk-sumo approved these changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@perk-sumo perk-sumo left a comment

Choose a reason for hiding this comment

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

LGTM 👍
@frankreno I assume we don't need those?

frankreno
frankreno suggested changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@frankreno frankreno left a comment

Choose a reason for hiding this comment

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

As discussed today, rather than dropping all _bucket metrics, let's future proof this by modifying the keep block with the explicit set of metrics we want to keep.

samjsong
samjsong approved these changes Apr 29, 2020
View reviewed changes
Copy link
Contributor

@samjsong samjsong left a comment

Choose a reason for hiding this comment

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

I like the comment, much more readable :)

@sumo-drosiek
Copy link
Contributor Author

@frankreno Requested changes done, re-review please :)

frankreno
frankreno approved these changes Apr 30, 2020
View reviewed changes
Copy link
Contributor

@frankreno frankreno 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 love the new format, it would be great if we could do this for all metrics...but we can do that later.

@sumo-drosiek sumo-drosiek mentioned this pull request May 4, 2020
Closed
@sumo-drosiek sumo-drosiek merged commit f513d77 into master May 4, 2020
@sumo-drosiek sumo-drosiek deleted the drosiek-drop-histogram-metrics branch May 4, 2020 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frankreno frankreno frankreno approved these changes

@samjsong samjsong samjsong approved these changes

@perk-sumo perk-sumo perk-sumo approved these changes

@vsinghal13 vsinghal13 Awaiting requested review from vsinghal13

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@sumo-drosiek @frankreno @samjsong @perk-sumo