xds/internal/xdsclient: Add counter metrics for valid and invalid resource updates #8038
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8038 +/- ##
==========================================
- Coverage 82.42% 82.33% -0.09%
==========================================
Files 387 387
Lines 39042 39066 +24
==========================================
- Hits 32179 32165 -14
- Misses 5546 5576 +30
- Partials 1317 1325 +8
|
There was a problem hiding this comment.
Mostly LGTM, just a few small things.
But I'm also worried this will conflict with some changes @purnesh42H is working on?
|
Removing "Required Reporter Clarification" status since this PR might have to wait for #8050 |
zasweq
force-pushed
the
xds-client-metrics-count
branch
from
23257e2 to
17e74b9
Compare
There was a problem hiding this comment.
@purnesh42H should probably take a pass but this LGTM. Thanks @zasweq!
xds/internal/xdsclient/pool.go
Outdated
| "google.golang.org/grpc/internal/xds/bootstrap" | ||
| ) | ||
|
|
||
| var ( | ||
| // DefaultPool is the default pool for xDS clients. It is created at init | ||
| // time by reading bootstrap configuration from env vars. | ||
| DefaultPool *Pool | ||
| DefaultPool *Pool | ||
| xdsClientResourceUpdatesValidMetric = estats.RegisterInt64Count(estats.MetricDescriptor{ |
There was a problem hiding this comment.
I think the right place for the counters is clientimpl.go file as its not related to pool. In fact, I think we even DefaultPool should be in clientimpl.go
There was a problem hiding this comment.
Good point. Done. Left DefaultPool here to keep this PR focused. I think DefaultPool makes sense either way, where it is now keeps it closer to type Pool but whatever you think is best moving forward.
| if err != nil { | ||
| t.Fatalf("Failed to create bootstrap configuration: %v", err) | ||
| } | ||
| // Create an xDS client for use by the cluster_resolver LB policy. |
There was a problem hiding this comment.
is there any relevance of cluster_resolver LB policy for this test? Because we are only testing listener resource.
| onDone() | ||
| } | ||
|
|
||
| // TestResourceUpdateMetrics configures an xDS Client and provides a valid and |
There was a problem hiding this comment.
// TestResourceUpdateMetrics configures an xDS client, and a management server to send valid and
// invalid LDS updates, and verifies that the expected metrics for both good and bad
// updates are emitted.
|
These are the only two counters. The other three xDS Client are gauge metrics. For correctness reasons, we need to switch our synchronous gauges to asynchronous gauges before implementing those three. |
This PR adds counter metrics for the xDS Client, and unit tests. E2E tests still need to be written.
The gauge metrics are blocked on adding support for asynchronous gauges, and we also should move our synchronous gauges in RLS to asynchronous.
RELEASE NOTES: