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

Make it possible to configure a metrics port in pods #392

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

fdfzcq
Copy link
Contributor

@fdfzcq fdfzcq commented Mar 11, 2024

This PR currently includes changes from #390

We (Spotify) are currently using this framework to set up automated benchmarking for our internal gRPC. While we have metrics from the driver for performance comparison, we would like to scrape and see actual metrics from the client/server in many cases. This PR adds a configuration metricsPort for both client and server. A metrics port with configured port number is exposed in the pod if configured, otherwise no extra ports are exposed.

Copy link

linux-foundation-easycla bot commented Mar 11, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@fdfzcq fdfzcq changed the title Make it possible to configure and expose a metrics port in pods Make it possible to configure a metrics port in pods Mar 12, 2024
Makefile Outdated Show resolved Hide resolved
@fdfzcq fdfzcq force-pushed the add-support-for-metrics-port branch from ad5a816 to a209c7e Compare March 18, 2024 09:54
@paulosjca paulosjca self-assigned this Mar 18, 2024
@paulosjca paulosjca added enhancement New feature or request release notes: yes Indicates that PR needs to be in release notes labels Mar 18, 2024
@fdfzcq fdfzcq requested a review from paulosjca March 23, 2024 09:05
Expect(getNames(runContainer.Ports)).NotTo(ContainElement("metrics"))
})

It("does not expose the metrics port if not set", func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should probably be "exposes the metrics port if set".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed in next commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops sorry I missed this comment, thank you for fixing it! @paulosjca

@paulosjca paulosjca merged commit cc442a6 into grpc:master Apr 3, 2024
1 of 2 checks passed
paulosjca pushed a commit to paulosjca/test-infra that referenced this pull request Apr 8, 2024
paulosjca pushed a commit to paulosjca/test-infra that referenced this pull request Jun 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request release notes: yes Indicates that PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants