-
Notifications
You must be signed in to change notification settings - Fork 67
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/appsflyer #44
base: main
Are you sure you want to change the base?
Feat/appsflyer #44
Conversation
ingestr/src/appsflyer/_init_.py
Outdated
@dlt.resource(write_disposition="merge", merge_key="install_time") | ||
def campaigns() -> Iterable[TDataItem]: | ||
yield from client.fetch_campaigns(start_date, end_date) | ||
@dlt.resource(write_disposition="merge", merge_key="Install Time") |
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.
the key is supposed to be as the fields in JSON, why did you go with Install Time
instead of the previous version?
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.
Because "Install Time" is the JSON key not "install_time". My understanding was wrong.
ingestr/src/appsflyer/_init_.py
Outdated
def campaigns( | ||
updated=dlt.sources.incremental('["Install Time"]', start_date_obj.isoformat()), | ||
) -> Iterable[TDataItem]: | ||
current_start_time = datetime.fromisoformat(updated.start_value).date() |
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.
shouldn't this be updated.last_value
?
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.
While testing, both start_value and last_value were giving the same value, so I used start_value.
ingestr/src/appsflyer/_init_.py
Outdated
yield from client.fetch_campaigns( | ||
start_date=current_start_time.isoformat(), end_date=next_end_date.isoformat() | ||
) | ||
print(current_start_time, next_end_date) |
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 shouldn't print this
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.
Done
ingestr/src/appsflyer/_init_.py
Outdated
current_start_time = datetime.fromisoformat(updated.start_value).date() | ||
end_date_time = datetime.fromisoformat(end_date).date() | ||
|
||
while current_start_time < end_date_time: |
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.
I'd move this logic into the internal appsflyer client so that we don't have to repeat it
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.
Done
ingestr/src/appsflyer/client.py
Outdated
max_cohort_duration = 7 | ||
max_allowed_end_date = (datetime.now() - timedelta(days=max_cohort_duration)).strftime('%Y-%m-%d') | ||
adjusted_end_date = min(end_date, max_allowed_end_date) | ||
return self._fetch_data(start_date, adjusted_end_date, metrics=metrics) |
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.
why?
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 can only fetch cohort metric data up to last seven days. If a user tries to fetch cohort data for today, Appsflyer shows an error "429." To handle this, we limit the end date to 7 days before the current date. For example, if a user is trying to fetch data from 2024-09-10 to 2024-10-20, the data will be fetched from 2024-09-10 to 2024-10-13 instead. Otherwise, it will show an error, and we won't be able to fetch data (this issue occurs only in the campaigns resource).
The following changes are made in this PR: