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

[Metrics] Add metrics validation in integration test #413

Merged

Conversation

JeffLuoo
Copy link
Contributor

Start by adding request total metrics, more validation will be added in follow up.

#326

@k8s-ci-robot k8s-ci-robot requested a review from ahg-g February 26, 2025 18:31
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 26, 2025
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 26, 2025
Copy link

netlify bot commented Feb 26, 2025

Deploy Preview for gateway-api-inference-extension ready!

Name Link
🔨 Latest commit 5d78491
🔍 Latest deploy log https://app.netlify.com/sites/gateway-api-inference-extension/deploys/67c07cf15ae09b00083f80a6
😎 Deploy Preview https://deploy-preview-413--gateway-api-inference-extension.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@JeffLuoo JeffLuoo changed the title [Metrics ]Add metrics validation in integration test [Metrics] Add metrics validation in integration test Feb 26, 2025
@JeffLuoo
Copy link
Contributor Author

cc - @courageJ

@JeffLuoo JeffLuoo force-pushed the metrics-integration-test branch from 47382f1 to 80dcdc8 Compare February 26, 2025 18:41
@@ -57,7 +66,8 @@ import (
)

const (
port = runserver.DefaultGrpcPort
port = runserver.DefaultGrpcPort
metricsPort = 8888
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can we define a DefaultMetricsPort as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default port 9090 used by metrics is already in use in the integration test so I picked a random port..

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we should test the same metrics endpoint as in production. Can you add a TODO here to follow up on setting up correct auth in integration test and test the "real" metrics endpoint?

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 add a comment in #326 (comment) to make sure we don't close the issue until it is improved.

Start by adding request total metrics, more validation will be added in
follow up.

kubernetes-sigs#326
@JeffLuoo JeffLuoo force-pushed the metrics-integration-test branch from 80dcdc8 to 5d78491 Compare February 27, 2025 14:55
@liu-cong
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2025
@ahg-g
Copy link
Contributor

ahg-g commented Feb 27, 2025

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, JeffLuoo

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2025
@k8s-ci-robot k8s-ci-robot merged commit 29bf32d into kubernetes-sigs:main Feb 27, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants