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

Exclude namespace queries to fetch container metric data #1309

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

shreyabiradar07
Copy link
Contributor

@shreyabiradar07 shreyabiradar07 commented Oct 1, 2024

Description

This PR filters out

  • namespace queries

  • redundant maxDate query (already executed to fetch the interval start and end time before iterating through the list of metrics)

to fetch container metric data to generate recommendations for a container based experiment to run only the relevant queries and not overload the datasource while running multiple experiments.

Fixes #1308

Type of change

  • Bug fix
  • New feature
  • Docs update
  • Breaking change (What changes might users need to make in their application due to this PR?)
  • Requires DB changes

How has this been tested?

Tested by running local monitoring demos on ResourceHub cluster - ./local_monitoring_demo.sh -c openshift -i quay.io/shbirada/autotune_operator:filter-namespace-for-container-reco

pod logs with queries run for tfb-server and tfb-database containers - https://privatebin.corp.redhat.com/?d50352a3d066c26b#4mGGpw7q2TXbrF1pN6C7yUykuphywPSwstMez2RkKWvB

  • New Test X
  • Functional testsuite

Test Configuration

  • Kubernetes clusters tested on: Openshift (ResourceHub cluster)

Checklist 🎯

  • Followed coding guidelines
  • Comments added
  • Dependent changes merged
  • Documentation updated
  • Tests added or updated

Additional information

docker image - quay.io/shbirada/autotune_operator:filter-namespace-queries

@shreyabiradar07 shreyabiradar07 self-assigned this Oct 1, 2024
@shreyabiradar07 shreyabiradar07 added bug Something isn't working kruize-local Tag for mentioning all the PR's and issues raised which covers the kruize local monitoring usecase labels Oct 1, 2024
@shreyabiradar07 shreyabiradar07 added this to the Kruize 0.0.26 Release milestone Oct 1, 2024
Copy link
Contributor

@msvinaykumar msvinaykumar left a comment

Choose a reason for hiding this comment

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

Shekar also implemented similar filtering. Can't we create a common function and reference it statically?

Is it possible to design the metric profile with subsections to handle both namespace and container queries? In the future, we might need separate queries for different workloads, so it would be better to design the metric profile object with distinct and clear separations, rather than relying on 'startswith'.

@shreyabiradar07
Copy link
Contributor Author

Shekar also implemented similar filtering. Can't we create a common function and reference it statically?

Added a common function for filtering metrics in both the namespace and container experiment types

Copy link
Contributor

@msvinaykumar msvinaykumar left a comment

Choose a reason for hiding this comment

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

LGTM

@dinogun
Copy link
Contributor

dinogun commented Oct 4, 2024

@shreyabiradar07 Any reason why we are not using kubernetes_object to check if it is container or namespace as it is meant for exactly this purpose?

@shreyabiradar07
Copy link
Contributor Author

@dinogun updated the function to filter metrics based on experiment_type and kubernetes_object

Copy link
Contributor

@dinogun dinogun left a comment

Choose a reason for hiding this comment

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

LGTM

@dinogun dinogun merged commit 236279c into kruize:mvp_demo Oct 8, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working kruize-local Tag for mentioning all the PR's and issues raised which covers the kruize local monitoring usecase
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants