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

Adding JSON files for Prometheus queries for Runtimes. #209

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

KillianGolds
Copy link
Contributor

Description

RHOAIENG-7353
RHOAIENG-3052
Prereq for #204

How Has This Been Tested?

I tested each query against a deployment of each runtime mentioned in the commits. To ensure the correct metric is returned.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@VedantMahabaleshwarkar
Copy link
Contributor

@alexcreasy could you take a look at this PR and lgtm purely in terms of the structure of the json files and ensuring that it looks like what has been discussed.

Copy link

@alexcreasy alexcreasy left a comment

Choose a reason for hiding this comment

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

General format looks good - raised a couple of points here.

@@ -0,0 +1,55 @@
{
"metrics": {
"supported": true,

Choose a reason for hiding this comment

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

@VedantMahabaleshwarkar you mentioned this needed to be a string value rather than a boolean for technical reasons - is that the case? The dashboard is currently looking for string "true" || "false".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Alex, updated the bool to be a string in the second commit! It was an oversight on my part. Apologies.

]
},
{
"title": "Memory usage over a range of time per model deployment",

Choose a reason for hiding this comment

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

@VedantMahabaleshwarkar Have you got these titles OK'd with UXD? They look extremely long and if we're moving to a model of displaying these strings directly in the dashboard they have to be approved.

This point doesn't need to hold up this PR but it's something that will need to be taken care of before it can be signed off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Alex, matched the titles to those that would be displayed by modelmesh in the second commit. Let me know if this is more suitable!

Signed-off-by: Killian Golds <kgolds@redhat.com>

Updated Query titles and fixed Memory Utilization metric query.
Copy link
Contributor

@VedantMahabaleshwarkar VedantMahabaleshwarkar left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

openshift-ci bot commented Jun 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KillianGolds, VedantMahabaleshwarkar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [VedantMahabaleshwarkar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 4ff41e3 into opendatahub-io:main Jun 17, 2024
5 checks passed
This pull request was closed.
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.

3 participants