-
Notifications
You must be signed in to change notification settings - Fork 4.6k
xds/internal/client: Add async gauge metrics for Connected and Resources (A78) #8807
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,4 +109,23 @@ type MetricsReporter interface { | |
| // Each client will produce different metrics. Please see the client's | ||
| // documentation for a list of possible metrics events. | ||
| ReportMetric(metric any) | ||
|
|
||
| // RegisterAsyncReporter registers a reporter to produce metric values for | ||
| // only the listed descriptors. The returned function must be called when | ||
| // the metrics are no longer needed, which will remove the reporter. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Does this guarantee that once the returned cancel func is called, the FYI: Even though this is an internal package, this xDS client is used by non-grpc folks, and we might fork this off to a separate repo or sub-module at some point in time. So, the requirements for docstrings here should be the same as that of any public APIs in our repo. |
||
| RegisterAsyncReporter(reporter AsyncReporter) func() | ||
| } | ||
|
|
||
| // AsyncReporter is an interface for types that record metrics asynchronously. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Can this line be replaced with: The fact that it is an interface is clear by the definition of the type. So, it need not be specified in the docstring. |
||
| // Implementations must be concurrent-safe. | ||
| type AsyncReporter interface { | ||
| // Report records metric values using the provided recorder. | ||
| Report(AsyncMetricsRecorder) error | ||
| } | ||
|
|
||
| // AsyncMetricsRecorder is a recorder for async metrics. | ||
| type AsyncMetricsRecorder interface { | ||
| // ReportMetric reports a metric. The metric will be one of the predefined | ||
| // set of types in the metrics.go file. | ||
| ReportMetric(metric any) | ||
|
Comment on lines
+128
to
+130
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if this method is called and passed a metric that is not supposed to be reported asynchronously?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,3 +40,18 @@ type ResourceUpdateInvalid struct { | |
| type ServerFailure struct { | ||
| ServerURI string | ||
| } | ||
|
|
||
| // XDSClientConnected reports the connectivity state of the xDS stream. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: s/xDS stream/ADS stream/ FYI: The xDS client establishes two streams, ADS and LRS. |
||
| // Value is 1 if connected, 0 otherwise. | ||
| type XDSClientConnected struct { | ||
| ServerURI string | ||
| Value int64 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this not be of type |
||
| } | ||
|
|
||
| // XDSClientResourceStats reports the number of resources currently cached. | ||
| type XDSClientResourceStats struct { | ||
| Authority string | ||
| ResourceType string | ||
| CacheState string | ||
| Count int64 | ||
|
Comment on lines
+53
to
+56
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand that the previously defined metric structs did not document the fields. But I feel we should do one of the following:
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these descriptors listed? Can that be specified in this docstring?