Conversation
|
This leaks PB types to the users, but this PR is only improving the typing of what users receive, it doesn't change any behaviour. But now that we have types, we can talk about improving the design. |
There was a problem hiding this comment.
Pull request overview
This PR improves the type-safety of samples produced by the Reporting API client by replacing dynamic getattr-based access patterns with strongly-typed structures, so MetricSample instances reflect the real value types coming from protobuf telemetry/state data.
Changes:
- Expand
MetricSampletyping (component ID and value union types) to reflect actual protobuf-derived data. - Refactor
GenericDataBatchto use typedProtocols + generics, replacing attribute-name strings with typed fetcher callables. - Replace multiple
getattr(...)usages in telemetry/state iteration with direct, typed attribute access.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| items = self.items_fetcher(self._data_pb) | ||
|
|
||
| for item in items: | ||
| cid = getattr(item, self.id_attr) | ||
| for sample in getattr(item, "metric_samples", []): | ||
| cid = self.id_fetcher(item) | ||
| for sample in item.metric_samples: |
There was a problem hiding this comment.
GenericDataBatch.__iter__() now relies on direct attribute access (item.metric_samples, item.state_snapshots, etc.). There are no unit tests covering iteration/unrolling behavior (only is_empty() is tested), so regressions here would be easy to miss. Consider adding a test that constructs a minimal batch (mock/proto) and asserts the yielded MetricSamples for both metric samples and state snapshots.
shsms
force-pushed
the
typing-improvements
branch
from
14a95e7 to
19169dd
Compare
Earlier there were a number of
getattrthat was hiding the actual types of values going intoMetricSamples. This PR replaces all such instances, so that only strongly typed values go intoMetricSamples.