Skip to content

Added metrics for restclient and workqueue#5731

Merged
arkodg merged 1 commit intoenvoyproxy:mainfrom
Inode1:feat-more-controller-metrics
Apr 24, 2025
Merged

Added metrics for restclient and workqueue#5731
arkodg merged 1 commit intoenvoyproxy:mainfrom
Inode1:feat-more-controller-metrics

Conversation

@Inode1
Copy link
Contributor

@Inode1 Inode1 commented Apr 14, 2025

This PR introduces additional metrics to improve observability around the Envoy Gateway, specifically focusing on the REST client and the workqueue.

Motivation
There are currently several issues that limit our ability to effectively monitor and troubleshoot the system:

Lack of Metrics: The limited set of existing metrics makes it difficult to gain insights into the performance and health of the Envoy Gateway. This PR enhances observability and makes it easier to identify and diagnose performance issues—particularly those related to the workqueue.

Workqueue Metrics Conflict: Metrics for the workqueue are registered both by controller-runtime and k8s.io/component-base via init() functions. However, only one of these libraries can successfully register the metrics. In our setup, k8s.io/component-base wins, which results in the metrics from controller-runtime not being exposed at all.

This PR resolves these issues by explicitly adding and managing the necessary metrics, ensuring better visibility into queue behavior and client interactions.

Release Notes: Yes

@Inode1 Inode1 requested a review from a team as a code owner April 14, 2025 11:31
@Inode1 Inode1 force-pushed the feat-more-controller-metrics branch 5 times, most recently from 97187d2 to 07f64a6 Compare April 15, 2025 08:43
@zirain
Copy link
Member

zirain commented Apr 15, 2025

can you add a test/e2e to make sure it worked as expected.

@codecov
Copy link

codecov bot commented Apr 15, 2025

Codecov Report

Attention: Patch coverage is 15.00000% with 51 lines in your changes missing coverage. Please review.

Project coverage is 65.27%. Comparing base (40fc25d) to head (af16e79).
Report is 41 commits behind head on main.

Files with missing lines Patch % Lines
internal/metrics/restclient/metrics.go 0.00% 22 Missing ⚠️
internal/metrics/workqueue/metrics.go 0.00% 22 Missing ⚠️
internal/metrics/register.go 0.00% 7 Missing ⚠️

❌ Your patch status has failed because the patch coverage (15.00%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5731      +/-   ##
==========================================
+ Coverage   65.21%   65.27%   +0.05%     
==========================================
  Files         214      217       +3     
  Lines       34321    34666     +345     
==========================================
+ Hits        22384    22628     +244     
- Misses      10584    10660      +76     
- Partials     1353     1378      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Inode1 Inode1 force-pushed the feat-more-controller-metrics branch from 07f64a6 to dab49f3 Compare April 15, 2025 15:08
@Inode1
Copy link
Contributor Author

Inode1 commented Apr 15, 2025

/retest

@Inode1 Inode1 force-pushed the feat-more-controller-metrics branch from dab49f3 to 8e86705 Compare April 16, 2025 05:07
@Inode1 Inode1 requested a review from zirain April 17, 2025 12:33
@zirain zirain force-pushed the feat-more-controller-metrics branch from 8e86705 to 10d793f Compare April 17, 2025 12:40
@Inode1 Inode1 force-pushed the feat-more-controller-metrics branch from 10d793f to eeba96d Compare April 17, 2025 13:50
Signed-off-by: Ivan Makarychev <makarichev.ivan@gmail.com>
@Inode1 Inode1 force-pushed the feat-more-controller-metrics branch from eeba96d to af16e79 Compare April 21, 2025 19:29
@Inode1 Inode1 requested a review from zirain April 21, 2025 19:32
@Inode1
Copy link
Contributor Author

Inode1 commented Apr 23, 2025

@zirain, could you take a look?

"sigs.k8s.io/controller-runtime/pkg/metrics"
)

// This file is copied and adapted from k8s.io/component-base/metrics/prometheus/workqueue
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@arkodg arkodg Apr 23, 2025

Choose a reason for hiding this comment

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

reread the title description, trying to better understand the conflict

Copy link
Contributor Author

@Inode1 Inode1 Apr 24, 2025

Choose a reason for hiding this comment

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

Sure, this and this both attempt to initialize workqueue metrics. But only one provider can be registered — enforced here in client-go. Component-base wins in our case. EG dont use PQ

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks

@arkodg arkodg merged commit bcb946e into envoyproxy:main Apr 24, 2025
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants