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

feat(RHTAPWATCH-570): Update exporters code with actual metrics #174

Merged
merged 1 commit into from
Feb 15, 2024
Merged

feat(RHTAPWATCH-570): Update exporters code with actual metrics #174

merged 1 commit into from
Feb 15, 2024

Conversation

hmariset
Copy link
Contributor

Update the dsexporter code and related unit tests with actual metrics.

@hmariset hmariset marked this pull request as draft January 19, 2024 18:33
amisstea
amisstea reviewed Jan 24, 2024
View reviewed changes
exporters/dsexporter/dsexporter.go Outdated Show resolved Hide resolved
exporters/dsexporter/dsexporter.go Outdated Show resolved Hide resolved
@hmariset hmariset force-pushed the exporter_update branch 3 times, most recently from 972d314 to 4ab1975 Compare January 30, 2024 12:38
@hmariset hmariset marked this pull request as ready for review January 30, 2024 12:39
yftacherzog
yftacherzog requested changes Jan 30, 2024
View reviewed changes
Copy link
Member

@yftacherzog yftacherzog left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Go checks (and others) are failing.

exporters/dsexporter/dsexporter_test.go Outdated Show resolved Hide resolved
@hmariset hmariset force-pushed the exporter_update branch 3 times, most recently from ca50e57 to f830b51 Compare January 30, 2024 19:08
@hmariset
Copy link
Contributor Author

/retest

@hmariset hmariset force-pushed the exporter_update branch 3 times, most recently from 2ac8ebf to 6ae759b Compare January 31, 2024 16:17
amisstea
amisstea requested changes Feb 1, 2024
View reviewed changes
exporters/dsexporter/dsexporter_test.go Outdated Show resolved Hide resolved
exporters/dsexporter/dsexporter.go Outdated Show resolved Hide resolved
@hmariset hmariset force-pushed the exporter_update branch 2 times, most recently from 3e15e1e to a263ebd Compare February 2, 2024 16:01
@amisstea amisstea mentioned this pull request Feb 12, 2024
Merged
@hmariset hmariset force-pushed the exporter_update branch 3 times, most recently from 636eb86 to 258993f Compare February 14, 2024 13:46
@hmariset
Copy link
Contributor Author

@yftacherzog @avi-biton @amisstea updated the code. Please review again.

@yftacherzog
Copy link
Member

@yftacherzog @avi-biton @amisstea updated the code. Please review again.

Did you confirm that this code can be deployed to the cluster at its current state, and Prometheus is scraping it correctly? I.e. you are able to query for the new metric and if you kill the grafana operator/service the metric reflects that.

@hmariset
Copy link
Contributor Author

@yftacherzog @avi-biton @amisstea updated the code. Please review again.

Did you confirm that this code can be deployed to the cluster at its current state, and Prometheus is scraping it correctly? I.e. you are able to query for the new metric and if you kill the grafana operator/service the metric reflects that.

@yftacherzog Yes, Prometheus is scraping correctly. I'm able to query the new metric and I also modified the metric to reflect 0 when grafana operator is killed.

@yftacherzog
Copy link
Member

@yftacherzog @avi-biton @amisstea updated the code. Please review again.

Did you confirm that this code can be deployed to the cluster at its current state, and Prometheus is scraping it correctly? I.e. you are able to query for the new metric and if you kill the grafana operator/service the metric reflects that.

@yftacherzog Yes, Prometheus is scraping correctly. I'm able to query the new metric and I also modified the metric to reflect 0 when grafana operator is killed.

I'm asking whether you confirmed it actually shows 0 correctly.

@hmariset
feat(RHTAPWATCH-570): Update exporters code with actual metrics
abaa3af 
Update the dsexporter code and related unit tests with
actual metrics.

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

@yftacherzog @avi-biton @amisstea updated the code. Please review again.

Did you confirm that this code can be deployed to the cluster at its current state, and Prometheus is scraping it correctly? I.e. you are able to query for the new metric and if you kill the grafana operator/service the metric reflects that.

@yftacherzog Yes, Prometheus is scraping correctly. I'm able to query the new metric and I also modified the metric to reflect 0 when grafana operator is killed.

I'm asking whether you confirmed it actually shows 0 correctly.

I confirm it shows 0 when I killed the grafana operator/service.

amisstea
amisstea approved these changes Feb 14, 2024
View reviewed changes
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

yftacherzog
yftacherzog approved these changes Feb 15, 2024
View reviewed changes
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.

lgtm

avi-biton
avi-biton approved these changes Feb 15, 2024
View reviewed changes
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

@yftacherzog yftacherzog merged commit 31b1462 into redhat-appstudio:main Feb 15, 2024
2 checks passed
@amisstea amisstea mentioned this pull request Feb 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yftacherzog yftacherzog yftacherzog approved these changes

@amisstea amisstea amisstea approved these changes

@avi-biton avi-biton avi-biton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hmariset @yftacherzog @amisstea @avi-biton