-
Notifications
You must be signed in to change notification settings - Fork 401
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(analytics): Add command to migrate analytics data to pg #3981
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Uffizzi Preview |
785f0b8
to
e5a68ad
Compare
e5a68ad
to
fe83bfd
Compare
42280b3
to
b9b4d59
Compare
Add command to migrate feature analytics data from influxdb to postgres
b9b4d59
to
e4ec116
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3981 +/- ##
========================================
Coverage 96.41% 96.42%
========================================
Files 1145 1148 +3
Lines 37303 37430 +127
========================================
+ Hits 35967 36092 +125
- Misses 1336 1338 +2 ☔ 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.
Some minor nits, but overall looks great.
api/app_analytics/migrate_to_pg.py
Outdated
from app_analytics.models import FeatureEvaluationBucket | ||
|
||
|
||
def migrate_feature_evaluations(migrate_till: int = 30): |
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.
def migrate_feature_evaluations(migrate_till: int = 30): | |
def migrate_feature_evaluations(migrate_till: int = 30) -> None: |
reason="Skip test if analytics database is not configured", | ||
) | ||
@pytest.mark.django_db(databases=["analytics", "default"]) | ||
def test_migrate_feature_evaluations(mocker: MockerFixture): |
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.
def test_migrate_feature_evaluations(mocker: MockerFixture): | |
def test_migrate_feature_evaluations(mocker: MockerFixture) -> None: |
def test_migrate_feature_evaluations(mocker: MockerFixture): | ||
# Given | ||
feature_name = "test_feature_one" | ||
environment_id = "1" |
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 feels odd to me that environment_id
is set to a string here.
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, but unfortunately this is the data type our functions expects (historical reasons)
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
Add command to migrate feature analytics data from InfluxDB to Postgres
How did you test this code?
Tested manually with an external InfluxDB and added a unit test.