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

recommender: fix instrumentation and add tests #230

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

iksaif
Copy link
Collaborator

@iksaif iksaif commented Nov 7, 2024

Fix recommender metrics ("recommender" != "client")

@iksaif iksaif requested a review from a team as a code owner November 7, 2024 10:36
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request does not contain a valid label. Please add one of the following labels: bug, enhancement, refactoring, documentation, tooling, dependencies

@iksaif iksaif added the bug Something isn't working label Nov 7, 2024
@iksaif iksaif added this to the 0.8.0 milestone Nov 7, 2024
github-actions[bot]
github-actions bot previously approved these changes Nov 7, 2024
github-actions[bot]
github-actions bot previously approved these changes Nov 7, 2024
github-actions[bot]
github-actions bot previously approved these changes Nov 7, 2024
@@ -29,7 +29,10 @@ const (
transitionPromLabel = "transition"
lifecycleStatus = "lifecycle_status"
monitorName = "monitor_name"
monitorNamespace = "monotor_namespace"
monitorNamespace = "monitor_namespace"
clientPromLabel = "client"
Copy link
Contributor

@vboulineau vboulineau Nov 7, 2024

Choose a reason for hiding this comment

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

The recommender or target/target_url name was perhaps more meaningful. As it's a not generic HTTP Client but only dedicated to call recommenders.

Copy link
Collaborator Author

@iksaif iksaif Nov 7, 2024

Choose a reason for hiding this comment

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

sure, but these metrics could be used for any http call (now that they are in "metrics") - and client contains the url of the recommender when it's a recommender

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair point, the metrics should be more specific then I think, but LGTM

@iksaif iksaif merged commit 3d4f969 into main Nov 8, 2024
23 checks passed
@iksaif iksaif deleted the corentin.chary/replica-recommender-3 branch November 8, 2024 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants