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

add trigger name as a label in Prometheus metrics #3650

Merged
merged 2 commits into from
Oct 27, 2022
Merged

add trigger name as a label in Prometheus metrics #3650

merged 2 commits into from
Oct 27, 2022

Conversation

cdlliuy
Copy link
Contributor

@cdlliuy cdlliuy commented Sep 8, 2022

This PR is used to resolve #3588

  • use the existing Trigger.Name field as the scalerName
  • add scalerName to prometheus metric as a label to facilitate the grafana dashboard display

see below:
image

@cdlliuy cdlliuy requested a review from a team as a code owner September 8, 2022 03:34
@cdlliuy cdlliuy mentioned this pull request Sep 8, 2022
Closed
JorTurFer
JorTurFer reviewed Sep 8, 2022
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! ❤️
I left some comments inline. Apart from them, could you open a PR against docs, explaining this new field?

pkg/metrics/prometheus_metrics.go Outdated Show resolved Hide resolved
pkg/scaling/cache/scalers_cache.go Show resolved Hide resolved
@JorTurFer JorTurFer requested a review from a team September 9, 2022 06:35
tomkerkhove
tomkerkhove suggested changes Sep 9, 2022
View reviewed changes
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Can you update our changelog please?

@JorTurFer
Copy link
Member

any update @cdlliuy ?
Could you update the changelog and solve the conflicts?

@cdlliuy cdlliuy force-pushed the ying/update_prometheus_name branch from 531c21e to 173d07f Compare October 9, 2022 09:06
@cdlliuy
Copy link
Contributor Author

cdlliuy commented Oct 9, 2022

any update @cdlliuy ? Could you update the changelog and solve the conflicts?

Done. Thanks for the reminder!

tomkerkhove
tomkerkhove approved these changes Oct 10, 2022
View reviewed changes
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

LTGM

@cdlliuy
Copy link
Contributor Author

cdlliuy commented Oct 11, 2022 •
edited
Loading

@tomkerkhove , can you help me to understand why "e2e-tests / e2e tests (pull_request_target) " is queued but not run? Thanks a lot!

@tomkerkhove
Copy link
Member

It's waiting for the label to be added on passing AFAIK.

@tomkerkhove
Copy link
Member

tomkerkhove commented Oct 11, 2022 •
edited by github-actions bot
Loading

/run-e2e *
Update: You can check the progress here

@tomkerkhove
Copy link
Member

tomkerkhove commented Oct 14, 2022 •
edited by github-actions bot
Loading

/run-e2e *

Update: You can check the progress here

@JorTurFer
Copy link
Member

JorTurFer commented Oct 14, 2022 •
edited by github-actions bot
Loading

/run-e2e
Update: You can check the progress here

@cdlliuy
Copy link
Contributor Author

cdlliuy commented Oct 27, 2022

@tomkerkhove @JorTurFer , I rebased to resolve the latest conflict on change.md, Can you help to run e2e and merge this PR?

@tomkerkhove
Copy link
Member

tomkerkhove commented Oct 27, 2022 •
edited by github-actions bot
Loading

/run-e2e
Update: You can check the progress here

@JorTurFer JorTurFer merged commit 83f882e into kedacore:main Oct 27, 2022
26tanishabanik pushed a commit to 26tanishabanik/keda that referenced this pull request Nov 1, 2022
@cdlliuy @26tanishabanik
add trigger name as a label in Prometheus metrics (kedacore#3650)
353d8d3 
* add trigger name as a label in prometheus metrics

Signed-off-by: Ying Liu <[email protected]>
Signed-off-by: 26tanishabanik <[email protected]>
@JorTurFer
Copy link
Member

Hello @cdlliuy ,
I didn't remember to request the documentation about this feature. It's true that the parameter was already there but it wasn't used either documented.
Could you open a PR in keda-docs repo documenting it?

@zroubalik zroubalik mentioned this pull request Nov 7, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JorTurFer JorTurFer JorTurFer approved these changes

@tomkerkhove tomkerkhove tomkerkhove 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.

Same metric name with multiple scaled object defined
3 participants
@cdlliuy @JorTurFer @tomkerkhove