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

Add stats to statsd storage #2638

Merged
merged 1 commit into from
Sep 11, 2020
Merged

Add stats to statsd storage #2638

merged 1 commit into from
Sep 11, 2020

Conversation

piowag
Copy link
Contributor

@piowag piowag commented Aug 7, 2020

Add stats to stdout storage: memory, hugetlb, perf, resctrl, referenced memory

Signed-off-by: Piotr Wagner <piotr.wagner@intel.com>
@k8s-ci-robot
Copy link
Collaborator

Hi @piowag. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dashpole
Copy link
Collaborator

dashpole commented Aug 7, 2020

/ok-to-test

@dashpole
Copy link
Collaborator

dashpole commented Aug 7, 2020

Same comment copied from the other PR:

This looks good. Do you think we have enough metrics here where we need to start respecting --ignore-metrics? I believe the current PR will just have zero values for disabled metrics, which is acceptable, but adds to the load for any backends that process the data.

@katarzyna-z
Copy link
Collaborator

Same comment copied from the other PR:

This looks good. Do you think we have enough metrics here where we need to start respecting --ignore-metrics? I believe the current PR will just have zero values for disabled metrics, which is acceptable, but adds to the load for any backends that process the data.

I think that it will be good to start respecting disable_metrics parameter also for other storages than Prometheus. My only doubt is how to clearly inform users about these changes. If we start now, for some of storages disable_metrics will be respected, for others will not be respected.
Maybe it will be good to add this information to description in help, for example:

-disable_metrics metrics
        comma-separated list of metrics to be disabled. Parameter is used by Prometheus and statsd storages. Options are 'accelerator', 'cpu_topology','disk', 'diskIO', 'network', 'tcp', 'udp', 'percpu', 'sched', 'process', 'hugetlb', 'referenced_memory', 'resctrl'. (default tcp,advtcp,sched,udp,process,hugetlb,referenced_memory,cpu_topology,resctrl)

@dashpole
Copy link
Collaborator

+1 to updating the flag description. If you'd prefer, we can only gate the new metrics with the respective disable_metric.

@dashpole dashpole self-assigned this Aug 13, 2020
Copy link
Collaborator

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

lgtm

@dashpole dashpole merged commit 67253bd into google:master Sep 11, 2020
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.

4 participants