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

chore(RHTAPWATCH-741): Add recording rules for exporter #205

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

hmariset
Copy link
Contributor

Add recording rules to translate the components existing metrics to expose availability.

@hmariset
Copy link
Contributor Author

/retest

Copy link
Collaborator

@amisstea amisstea left a comment

Choose a reason for hiding this comment

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

Do you plan to add some documentation in a followup PR (building upon #173)?

rhobs/recording/exporter_recording_rules.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@amisstea amisstea left a comment

Choose a reason for hiding this comment

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

LGTM

@hmariset
Copy link
Contributor Author

/retest

@hmariset hmariset force-pushed the recording-rules-exporter branch 9 times, most recently from f7bb121 to bd6fb50 Compare February 16, 2024 18:39
Copy link
Member

@yftacherzog yftacherzog left a comment

Choose a reason for hiding this comment

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

  • We need test cases for the recording rules. You can find references for recording rules test cases in our git history (we used to have a couple of recording rules in the past and you should be able to see references to their test cases).
  • I'm concerned for the fact that the metric name can change without us having to change the test cases (we might be missing test cases). Not sure if this PR is the place to address that, but please spend some time examining this and open a ticket to address/explore it.

@hmariset hmariset force-pushed the recording-rules-exporter branch 2 times, most recently from a50c91d to c95471e Compare February 19, 2024 16:39
@hmariset
Copy link
Contributor Author

  • We need test cases for the recording rules. You can find references for recording rules test cases in our git history (we used to have a couple of recording rules in the past and you should be able to see references to their test cases).
  • I'm concerned for the fact that the metric name can change without us having to change the test cases (we might be missing test cases). Not sure if this PR is the place to address that, but please spend some time examining this and open a ticket to address/explore it.

Added the recording rules test case. I'll open a new ticket for exploring the second question.

Add recording rules to translate the components existing
metrics to expose availability.

Signed-off-by: Homaja Marisetty <[email protected]>
@hmariset
Copy link
Contributor Author

@yftacherzog @amisstea I have updated the code. Please review again.

@yftacherzog
Copy link
Member

/retest

Copy link
Member

@yftacherzog yftacherzog left a comment

Choose a reason for hiding this comment

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

Looks good!
Once you have this one merged, make sure you create the necessary changes in app-interface and update the readme here with the instructions for how to update the recording rules references.

Copy link
Collaborator

@avi-biton avi-biton left a comment

Choose a reason for hiding this comment

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

/lgtm

@hmariset
Copy link
Contributor Author

Looks good! Once you have this one merged, make sure you create the necessary changes in app-interface and update the readme here with the instructions for how to update the recording rules references.

Sure Yftach! I already created the MR for app-interface (https://gitlab.cee.redhat.com/service/app-interface/-/merge_requests/97295) and will update README as well. Thank you!

@hmariset hmariset merged commit 780ac90 into redhat-appstudio:main Feb 21, 2024
2 checks passed
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.

4 participants