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

Integrate MetadataProfile with CreateExperiment API #1496

Merged
merged 14 commits into from
Mar 21, 2025
Merged

Integrate MetadataProfile with CreateExperiment API #1496

merged 14 commits into from
Mar 21, 2025
+246 −4

Conversation

shreyabiradar07
Copy link
Contributor

@shreyabiradar07 shreyabiradar07 commented Feb 4, 2025 •
edited
Loading

Description

This PR includes the following changes:

  • Adds new field: metadata_profile in KruizeLMExperimentEntry.java local monitoring experiment.
  • Adds MetadataProfile CRD context
  • Integrates metadata_profile field with CreateExperiment API for local monitoring

Note: PR #1543 has corresponding test and documentation changes.

Type of change

  • Bug fix
  • New feature
  • Docs update
  • Breaking change (What changes might users need to make in their application due to this PR?)
  • Requires DB changes

How has this been tested?

Please describe the tests that were run to verify your changes and steps to reproduce. Please specify any test configuration required.

  • New Test X
  • Functional testsuite

Test Configuration

  • Kubernetes clusters tested on:

Checklist 🎯

  • Followed coding guidelines
  • Comments added
  • Dependent changes merged
  • Documentation updated
  • Tests added or updated

Additional information

docker image: quay.io/shbirada/integrate_create_exp:v1

Sorry, something went wrong.

@shreyabiradar07 shreyabiradar07 self-assigned this Feb 4, 2025
@shreyabiradar07 shreyabiradar07 added this to the Kruize 0.5 Release milestone Feb 6, 2025
chandrams
chandrams reviewed Mar 7, 2025
View reviewed changes
src/main/java/com/autotune/operator/KruizeOperator.java Outdated
@@ -487,6 +505,44 @@ public static String setDefaultMetricProfile(SloInfo sloInfo, String mode, Strin
return metricProfile.getName();
}


public static String setDefaultMetadataProfile() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this function still relevant as we are setting up default profiles through jsons in the other PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to verify this as the default profiles through jsons are set up only for ROS and not for local monitoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed setDefaultMetadataProfile() as this is not required/relevant for local monitoring

@shreyabiradar07 shreyabiradar07 marked this pull request as ready for review March 13, 2025 09:50
@shreyabiradar07 shreyabiradar07 force-pushed the metadataProfile-createExp-integration branch from 2847bb8 to c832ff0 Compare March 13, 2025 10:25
@shreyabiradar07 shreyabiradar07 mentioned this pull request Mar 19, 2025
Merged
12 tasks
khansaad
khansaad requested changes Mar 19, 2025
View reviewed changes
khansaad
khansaad approved these changes Mar 19, 2025
View reviewed changes
Copy link
Contributor

@khansaad khansaad left a comment

Choose a reason for hiding this comment

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

LGTM!

chandrams
chandrams approved these changes Mar 19, 2025
View reviewed changes
Copy link
Contributor

@chandrams chandrams left a comment

Choose a reason for hiding this comment

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

LGTM

@chandrams
Copy link
Contributor

@shreyabiradar07 - Are we updating the demos with the changes done in all PRs related to default metric, metadata profiles?

msvinaykumar
msvinaykumar approved these changes Mar 20, 2025
View reviewed changes
Copy link
Contributor

@msvinaykumar msvinaykumar left a comment

Choose a reason for hiding this comment

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

LGTM

@shreyabiradar07
Copy link
Contributor Author

@shreyabiradar07 - Are we updating the demos with the changes done in all PRs related to default metric, metadata profiles?

@chandrams kruize-demos PR 128, has the corresponding default metric, metadata profiles and integration changes with create experiment JSON.

@dinogun
Copy link
Contributor

dinogun commented Mar 21, 2025

@shreyabiradar07 Please fix conflicts

@shreyabiradar07
Copy link
Contributor Author

@shreyabiradar07 Please fix conflicts

Resolved the conflicts

@dinogun dinogun merged commit c45bf18 into kruize:mvp_demo Mar 21, 2025
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chandrams chandrams

@khansaad khansaad

@msvinaykumar msvinaykumar

Assignees

@shreyabiradar07 shreyabiradar07

Labels
enhancement New feature or request local_monitoring
Projects
Monitoring
Status: Done
Milestone
Kruize 0.5 Release
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@shreyabiradar07 @chandrams @dinogun @khansaad @msvinaykumar