feat(metering): add Python SDK support for the Metering API#2663
feat(metering): add Python SDK support for the Metering API#2663krishna1633 wants to merge 10 commits into
Conversation
Adds `client.metering` with `retrieve`, `retrieve_multiple`, and `list` methods backed by the alpha Metering API endpoints.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2663 +/- ##
==========================================
+ Coverage 93.11% 93.59% +0.47%
==========================================
Files 490 497 +7
Lines 49869 50222 +353
==========================================
+ Hits 46437 47003 +566
+ Misses 3432 3219 -213
🚀 New features to boost your workflow:
|
…ump in MeteringData
There was a problem hiding this comment.
Code Review
This pull request introduces the Metering API to the SDK, adding support for listing and retrieving consumption data through both asynchronous and synchronous clients, along with corresponding data classes, documentation, and tests. The review feedback recommends adding defensive input validation to retrieve_multiple to handle empty lists, single strings, or invalid element types, as well as adding unit tests to verify these defensive checks.
haakonvt
left a comment
There was a problem hiding this comment.
A few comments - did not make it all the way through 😄 A lot looks great btw, much nits here:
| def _alpha_headers(self) -> dict[str, str]: | ||
| return {"cdf-version": f"{self._config.api_subversion}-alpha"} |
There was a problem hiding this comment.
Lets move this to utilities + make it check that "alpha" is not already in self._config.api_subversion
There was a problem hiding this comment.
...and please, change existing uses like headers = {"cdf-version": f"{self._config.api_subversion}-alpha"} ❤️
There was a problem hiding this comment.
Done — moved to BasicAsyncAPIClient._alpha_version_header() with a double-alpha guard. Updated LimitsAPI inline uses as well.
| start: int | None = None, | ||
| end: int | None = None, |
There was a problem hiding this comment.
See what e.g. DatapointsAPI supports for start/end (at least tz aware datetime)
There was a problem hiding this comment.
Done — start and end now accept int (epoch ms), datetime, or relative strings like "2w-ago". Converted to epoch ms via timestamp_to_ms before hitting the API.
| For instance ``atlas.monthly_ai_tokens`` is the id of the ``atlas`` service metering ``monthly_ai_tokens``. | ||
| Service and metering names are always in ``lower_snake_case``. | ||
| start (int | None): Start timestamp (inclusive) for historical data, in milliseconds since epoch. | ||
| **Must be provided together with** ``number_of_datapoints`` to get time-series data. |
There was a problem hiding this comment.
Must be provided together with
Should you validate this in the code?
There was a problem hiding this comment.
Adding a client-side guard would diverge from the SDK pattern of not duplicating server-enforced rules. Caller needs an error to know. It may not be concerned if error is from sdk or API as long as it is clear.
Per the server spec, providing only start without numberOfDatapoints silently returns metadata (server default is 0) — this is documented, expected behavior. The docstring already states Must be provided together with.
There was a problem hiding this comment.
We regularly duplicate such logic in the SDK for immediate user feedback, so please add here. I agree that for the number of allowed datapoints, that might change in the future so it should not be validated.
| params=params, | ||
| ) | ||
|
|
||
| async def retrieve_multiple( |
There was a problem hiding this comment.
We don't use retrieve_multiple anymore; it is from the time when we did not have typing.overload 😄
There was a problem hiding this comment.
Done — removed retrieve_multiple, retrieve now uses @overload (single str → MeteringData | None, list → MeteringDataList). Same pattern as UnitsAPI
|
|
||
| Args: | ||
| filter (Prefix | None): Optional ``Prefix`` filter to apply on the ``meterId`` property (only ``Prefix`` filters are supported). | ||
| limit (int | None): Maximum number of meters to return. Defaults to 1000. Set to ``None`` or ``-1`` to return all meters. |
There was a problem hiding this comment.
Fixed — docstring now correctly says 25 (DEFAULT_LIMIT_READ)
| other_params = self._time_range_params(start, end, number_of_datapoints) or None | ||
|
|
||
| return await self._list( | ||
| method="GET" if filter is None else "POST", |
There was a problem hiding this comment.
Why can't you just always use GET here? Passing no filter is kind of like passing a filter matching everything, right?
method="GET" if filter is None else "POST",There was a problem hiding this comment.
did not follow.
API supports both . GET is used for the no-filter case since the endpoint doesn't accept a body.
There was a problem hiding this comment.
Then why can't you always use POST?
| def __eq__(self, other: Any) -> bool: | ||
| return isinstance(other, MeteringDataPoint) and self.dump() == other.dump() |
There was a problem hiding this comment.
Why have you implemented dunder eq?
There was a problem hiding this comment.
Done — removed. CogniteResource provides eq via dump()
| from cognite.client.data_classes._base import CogniteResource, CogniteResourceList, basic_instance_dump | ||
|
|
||
|
|
||
| class MeteringDataPoint: |
There was a problem hiding this comment.
Should inherit from CogniteResource
| def _load(cls, resource: dict[str, Any]) -> MeteringDataPoint: | ||
| return cls(timestamp=resource["timestamp"], average=resource["average"]) | ||
|
|
||
| def dump(self) -> dict[str, Any]: |
There was a problem hiding this comment.
Missing "camel_case" arg (although it doesnt do anything here)
Sure. Once I address these comments, will ask you for another iteration so you can all the way through |
|
@haakonvt This is ready again |
haakonvt
left a comment
There was a problem hiding this comment.
Please resolve comments you have addressed
|
|
||
| def _alpha_version_header(self) -> dict[str, str]: | ||
| subversion = self._config.api_subversion | ||
| version = subversion if subversion.endswith("-alpha") else subversion + "-alpha" |
There was a problem hiding this comment.
Just to be on the safe side
| version = subversion if subversion.endswith("-alpha") else subversion + "-alpha" | |
| version = subversion if "alpha" in subversion else subversion + "-alpha" |
| self._warning.warn() | ||
|
|
||
| headers = {"cdf-version": f"{self._config.api_subversion}-alpha"} | ||
| headers = self._alpha_version_header() |
| self._warning.warn() | ||
| if isinstance(id, str): | ||
| params = self._time_range_params(start, end, number_of_datapoints) or None | ||
| return await self._retrieve( | ||
| identifier=MeterId(id), | ||
| cls=MeteringData, | ||
| headers=self._alpha_version_header(), | ||
| params=params, | ||
| ) | ||
| body: dict[str, Any] = {"items": [MeterId(id_).as_dict() for id_ in id]} | ||
| body.update(self._time_range_params(start, end, number_of_datapoints)) | ||
| res = await self._post( | ||
| url_path=self._RESOURCE_PATH + "/byids", | ||
| json=body, | ||
| headers=self._alpha_version_header(), | ||
| semaphore=self._get_semaphore("read"), | ||
| ) | ||
| return MeteringDataList._load(res.json()["items"])._maybe_set_client_ref(self._cognite_client) |
There was a problem hiding this comment.
See if you can use _retrieve_multiple
| other_params = self._time_range_params(start, end, number_of_datapoints) or None | ||
|
|
||
| return await self._list( | ||
| method="GET" if filter is None else "POST", |
There was a problem hiding this comment.
Then why can't you always use POST?
| res = cognite_client.metering.list() | ||
|
|
||
| assert isinstance(res, MeteringDataList) | ||
| assert all(meter.meter_id is not None for meter in res) |
There was a problem hiding this comment.
Don't use all in tests as they give false positives for empty (unless you first assert on non-empty) :
>>> all(x is None for x in())
True|
|
||
| assert isinstance(res, MeteringDataList) | ||
| assert len(res) == len(ids) | ||
| assert [m.meter_id for m in res] == ids |
| json=ATLAS_METER, | ||
| ) | ||
|
|
||
| meter_id = str(ATLAS_METER["meterId"]) |
| url_pattern = re.compile(re.escape(f"{metering_url}/{ATLAS_METER['meterId']}") + r"\?.*") | ||
| httpx_mock.add_response( | ||
| method="GET", | ||
| url=url_pattern, | ||
| status_code=200, | ||
| json=ATLAS_METER_WITH_DATA, | ||
| ) |
| assert f"start={timestamp_to_ms(start_dt)}" in request_url | ||
| assert f"end={timestamp_to_ms(end_dt)}" in request_url | ||
|
|
||
| def test_retrieve_with_relative_string_start( |
There was a problem hiding this comment.
So many tests are slightly similar here; please manually make a selection. Consider parametrize.
| start_val = int(request_url.split("start=")[1].split("&")[0]) | ||
| assert start_val > 0 | ||
|
|
||
| def test_dump_and_load_roundtrip(self) -> None: |
There was a problem hiding this comment.
Dump load roundtrips are automatically tested.
Description
Adds SDK support for the Metering API to retrieve consumption data for a CDF project. Follows the same pattern as the Limits API (#2436).
API spec:
versions/v1/metering.preproc.ymlKey Features
MeteringData,MeteringDataList,MeteringDataPointclient.metering:retrieve(id, start, end, number_of_datapoints)— GET/metering/meters/{meterId}retrieve_multiple(ids, start, end, number_of_datapoints)— POST/metering/meters/byidslist(filter, limit, start, end, number_of_datapoints)— GET/POST/metering/meters[/list]start+number_of_datapointstogether enable historical data)cdf-version: {api_subversion}-alphaheader (alpha API)Implementation Details
retrieve()— GET with optional query params for time rangeretrieve_multiple()— direct_postto/byids(custommeterIdfield, not standardid/externalId)list()— GET without filter, POST to/listwithPrefixfilter; time-range params go as query params (GET) or body (POST) viaother_paramsMeterIdidentifier class inutils/_identifier.py(mirrorsLimitId)sync-client-codegenhookChecklist
docs/source/metering.rst)"metering"added to idempotent POST whitelist intest_meta.pytest_api_client.py