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: support custom metrics for metrics adapter #3638

Merged
merged 1 commit into from
Jun 19, 2023
Merged

feat: support custom metrics for metrics adapter #3638

merged 1 commit into from
Jun 19, 2023

Conversation

chaunceyjiang
Copy link
Member

What type of PR is this?
/kind feature

What this PR does / why we need it:

support custom metrics for metrics adapter

Which issue(s) this PR fixes:
Part of #3632

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-metrics-adapter`: support custom metrics for metrics adapter

@karmada-bot karmada-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Jun 5, 2023
@karmada-bot karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 5, 2023
@chaunceyjiang chaunceyjiang force-pushed the custom-metrics branch 3 times, most recently from 29fa343 to 636c745 Compare June 5, 2023 17:10
@codecov-commenter
Copy link

Codecov Report

Merging #3638 (636c745) into master (bfb9bc3) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master    #3638   +/-   ##
=======================================
  Coverage   55.23%   55.24%           
=======================================
  Files         221      221           
  Lines       20831    20838    +7     
=======================================
+ Hits        11506    11511    +5     
- Misses       8715     8717    +2     
  Partials      610      610
Flag Coverage Δ
unittests 55.24% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 2 files with indirect coverage changes

@chaunceyjiang chaunceyjiang force-pushed the custom-metrics branch 2 times, most recently from 26ebaa3 to 3950e6e Compare June 7, 2023 06:48
@chaunceyjiang
Copy link
Member Author

image

@chaunceyjiang
Copy link
Member Author

/cc @jwcesign @Poor12 Please take a look.

@chaunceyjiang chaunceyjiang force-pushed the custom-metrics branch 3 times, most recently from 405434d to 7461bb3 Compare June 8, 2023 05:57
@chaunceyjiang chaunceyjiang marked this pull request as ready for review June 8, 2023 06:08
@karmada-bot karmada-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 8, 2023
@chaunceyjiang
Copy link
Member Author

/cc @Poor12 @jwcesign

@chaunceyjiang chaunceyjiang force-pushed the custom-metrics branch 2 times, most recently from ef8b8a3 to 6269e9a Compare June 8, 2023 11:08
@chaunceyjiang
Copy link
Member Author

/cc @Poor12 @jwcesign

jwcesign
jwcesign reviewed Jun 12, 2023
View reviewed changes
Copy link
Member

@jwcesign jwcesign left a comment

Choose a reason for hiding this comment

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

other lgtm

pkg/metricsadapter/provider/custommetrics.go Show resolved Hide resolved
pkg/metricsadapter/provider/custommetrics.go Outdated Show resolved Hide resolved
@chaunceyjiang
Copy link
Member Author

image

@jwcesign
Copy link
Member

Nice work!
/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 12, 2023
@jwcesign
Copy link
Member

cc @Poor12 @RainbowMango for checking

RainbowMango
RainbowMango reviewed Jun 13, 2023
View reviewed changes
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/assign
I'll look at it tomorrow.

@Poor12
Copy link
Member

Poor12 commented Jun 14, 2023

LGTM
It works well in my local env.

@jwcesign jwcesign mentioned this pull request Jun 16, 2023
Closed
@karmada-bot karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2023
RainbowMango
RainbowMango approved these changes Jun 16, 2023
View reviewed changes
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/approve

Leave LGTM to @jwcesign for final decision.

@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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:

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 16, 2023
@Poor12 Poor12 mentioned this pull request Jun 16, 2023
Merged
@chaunceyjiang
Copy link
Member Author

/cc @jwcesign

jwcesign
jwcesign reviewed Jun 19, 2023
View reviewed changes
Copy link
Member

@jwcesign jwcesign left a comment

Choose a reason for hiding this comment

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

other lgtm

pkg/metricsadapter/provider/custommetrics.go Show resolved Hide resolved
pkg/metricsadapter/provider/custommetrics.go Show resolved Hide resolved
@chaunceyjiang
feat: support custom metrics for metrics adapte
70e5260 
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
@jwcesign
Copy link
Member

jwcesign commented Jun 19, 2023 •
edited
Loading

Nice work!

@jwcesign
Copy link
Member

/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2023
@chaunceyjiang
Copy link
Member Author

/cc @RainbowMango Can you help re-trigger action ?

@RainbowMango
Copy link
Member

Done. This error will be ignored after #3674 .

@karmada-bot karmada-bot merged commit 0febf48 into karmada-io:master Jun 19, 2023
9 checks passed
@chaunceyjiang chaunceyjiang deleted the custom-metrics branch June 26, 2023 07:29
@chaosi-zju chaosi-zju mentioned this pull request Aug 16, 2023
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwcesign jwcesign jwcesign left review comments

@chaosi-zju chaosi-zju chaosi-zju left review comments

@RainbowMango RainbowMango RainbowMango approved these changes

@Garrybest Garrybest Awaiting requested review from Garrybest

@XiShanYongYe-Chang XiShanYongYe-Chang Awaiting requested review from XiShanYongYe-Chang

@Poor12 Poor12 Awaiting requested review from Poor12

Assignees

@RainbowMango RainbowMango

@jwcesign jwcesign

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@chaunceyjiang @codecov-commenter @jwcesign @Poor12 @karmada-bot @RainbowMango @chaosi-zju