-
Notifications
You must be signed in to change notification settings - Fork 447
feat: Backend for SDK metrics #5623
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: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
a36fe70
to
2b706ee
Compare
2b706ee
to
88498ed
Compare
88498ed
to
421522b
Compare
421522b
to
8664928
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5623 +/- ##
==========================================
+ Coverage 97.74% 97.80% +0.06%
==========================================
Files 1255 1247 -8
Lines 44500 44221 -279
==========================================
- Hits 43496 43251 -245
+ Misses 1004 970 -34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8664928
to
2a67741
Compare
2a67741
to
02c72e2
Compare
Docker builds report
|
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
if period in {constants.CURRENT_BILLING_PERIOD, constants.PREVIOUS_BILLING_PERIOD}: | ||
if not (sub_cache and sub_cache.is_billing_terms_dates_set()): | ||
raise NotFound("No billing periods found for this organisation.") | ||
|
||
if TYPE_CHECKING: | ||
assert sub_cache |
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.
Context aside, this feels a bit fragile. Can we make sure sub_cache
is not None before this assert
statement?
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.
We actually are doing it by raising NotFound
, but mypy is too dumb to understand that.
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 raises only if period
is found in that set, + if sub_cache.is_billing_terms_dates_set()
evaluates to true, which from here feels like sub_cache
could still be None
at L216. Perhaps tweaking the logical expression could help mypy? This is a nit unless there's actual risk of an type/attribute error.
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.
Nope, it raises if sub_cache
is None
. (sub_cache and sub_cache.is_billing_terms_dates_set())
evaluates to None
, and not (sub_cache and sub_cache.is_billing_terms_dates_set())
evaluates to 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.
Sorry, I'm slow today — will fix
Thanks for submitting a PR! Please check the boxes below:
docs/
if required so people know about the feature!Changes
Closes #5581.
Contributes to #2479.
This PR adds support for optional header tracking in the backend. It adds extensible, fully typed mechanism to support other optional labels for analytics in the future. The usage data and feature evaluation data routes are updated to return labeled data buckets. Additionally, numerous typing and code structure improvements are introduced to the analytics-related code base.
SDK API
All tracked SDK APIs now expect the following optional headers, translated to labels written to PostgreSQL or InfluxDB usage and evaluation data buckets:
Flagsmith-Application-Name
header:client_application_name
labelFlagsmith-Application-Version
header:client_application_version
labelRelevant caches and tasks are changed to properly compact the raw data, producing buckets with unique label sets.
Management API
The following API routes are updated to support labels:
/api/v1/organisations/{organisation_pk}/usage-data/
/api/v1/projects/{project_pk}/features/{id}/evaluation-data/
Previously, these routes were returning one bucket per day. Now, multiple buckets can be attributed to the same day, as long as they have different unique label sets. The label sets are returned under the
label
key for each bucket.Both of them now accept new query parameters:
client_application_name
client_application_version
to allow bucket filtering by label.
Task processing
Due to incompatible signature change, the
track_feature_evaluation
is decommissioned in favour of the newtrack_feature_evaluations_by_environment
task, which now supports labels included as part of feature evaluation data.How did you test this code?
Unit tests have been added and updated to reflect new behaviour. Manually with local PostgreSQL storage. Additional tests will be conducted in staging against InfluxDB Cloud to measure the performance.