-
Notifications
You must be signed in to change notification settings - Fork 7
Open
Labels
discussionThere are several possible ways to resolve this issue that need a discussionThere are several possible ways to resolve this issue that need a discussion
Description
Over the past few months, many parts of the metricq API gained type annotations that (imo) made the code easier to work with.
One thing that I keep stumbling upon is that for many RPC wrappers, the their return value is either not specified at all, or just described as "whatever JSON we got, converted to a dict".
Prime example, sink.subscribe:
metricq-python/metricq/sink.py
Lines 124 to 159 in 8e73ceb
| async def subscribe( | |
| self, | |
| metrics, | |
| expires: Union[None, int, float] = None, | |
| metadata: Optional[bool] = None, | |
| **kwargs, | |
| ): | |
| """Subscribe to a list of metrics. | |
| Args: | |
| metrics: names of the metrics to subscribe to | |
| expires: queue expiration time in seconds | |
| metadata: whether to return metric metadata in the response, defaults to ``True`` | |
| Returns: | |
| rpc response | |
| """ | |
| if self._data_queue is not None: | |
| kwargs.update(dataQueue=self._data_queue.name) | |
| if expires is not None: | |
| kwargs.update(expires=expires) | |
| if metadata is not None: | |
| kwargs.update(metadata=metadata) | |
| response = await self.rpc("sink.subscribe", metrics=metrics, **kwargs) | |
| self._subscribed_metrics.update(metrics) | |
| # Save the subscription RPC args in case we need to resubscribe (after a reconnect). | |
| self._subscribe_args = kwargs | |
| if self._data_queue is None: | |
| await self.sink_config(**response) | |
| return response |
Documentation is simply "returns: rpc response", not really informative.
The RPC spec tells us though that it contains useful metric data in response["metrics"].
My question(s):
- Should we have some nice wrapper types for RPC responses, at least where a response reaches API boundaries?
- Should there be proper type aliases that make working with such responses easier? (See e.g. the private alias
_GetMetricsResulthere) - Is it desirable to limit what info is leaked at the API boundary?
sink.subscribefor example also returns information about data server address and queue. I'd say the only data of value there is the metric metadata. The rest is handled by the implementation.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
discussionThere are several possible ways to resolve this issue that need a discussionThere are several possible ways to resolve this issue that need a discussion