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
feat: Add api usage metrics for different periods #3870
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Uffizzi Ephemeral Environment
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3870 +/- ##
==========================================
+ Coverage 96.18% 96.24% +0.06%
==========================================
Files 1141 1142 +1
Lines 36507 36687 +180
==========================================
+ Hits 35114 35310 +196
+ Misses 1393 1377 -16 ☔ View full report in Codecov by Sentry. |
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.
LGTM in general. Didn't manage to find any flaws in logic/architecture. Commented a couple minor issues/nitpicks. Requesting changes because some of the comments point out incorrect typing.
environment_id: int | None = None, | ||
project_id: int | None = None, | ||
period: ( | ||
Literal[ |
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.
It's nicer to avoid defining the type inline here, i.e.
UsageDataPeriod = Literal[
constants.CURRENT_BILLING_PERIOD,
constants.PREVIOUS_BILLING_PERIOD,
constants._90_DAY_PERIOD,
]
...
def get_usage_data(
...
period: UsageDataPeriod | None = None,
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.
Yeah I agree I'll do that when I update this PR.
period = serializers.RegexField( | ||
regex=r"^(%s)$" | ||
% "|".join( | ||
[ | ||
constants.CURRENT_BILLING_PERIOD, | ||
constants.PREVIOUS_BILLING_PERIOD, | ||
constants._90_DAY_PERIOD, | ||
] | ||
), | ||
required=False, | ||
) |
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.
When I offered to use RegexField
initially, I was thinking more of a ^\d+[d|h|m]$
regex, as I didn't know that the periods were meant to be predefined.
Now it seems more appropriate to use ChoiceField
instead (and if you define the literal type outside the method definition as suggested here, you'll be able to use typing.get_args
with it to provide the choices in a DRY manner).
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.
Yeah I should have gone with a ChoiceField
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
This provides the ability to get API usage data for different billing periods. The supported new periods are the current billing period (i.e., the first day of the billing period till now), the previous billing period, and a 90-day rolling period to the present.
Part of these changes involved with a refactoring of how code is called to influxdb. The former code incorrectly attributed
date_range
as if it weredate_start
with an inverse modifier. With the former code any callers that had set adate_stop
to anything other thannow()
the resultant range would be smaller than the interface contract implied. These issues have been addressed and subsequent calling methods have been updated.There was also an issue that cropped up with Pydantic and Freeze Gun, which has been temporarily solved with the addition of an auto-use function in
conftest.py
.How did you test this code?
Tested manually against production influxdb to verify results then also added 3 tests to ensure calling signatures to related influxdb functions.