-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
🚨🚨 Source Instagram: upgrade to v18 #33930
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
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.
This seems on the right track but I lack information to approve. Can you answer those questions?
airbyte-integrations/connectors/source-instagram/integration_tests/expected_records.jsonl
Show resolved
Hide resolved
@@ -143,10 +143,18 @@ def read_records( | |||
class UserLifetimeInsights(DatetimeTransformerMixin, InstagramStream): | |||
"""Docs: https://developers.facebook.com/docs/instagram-api/reference/ig-user/insights""" | |||
|
|||
primary_key = ["business_account_id", "metric", "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.
I don't have any sample response from the previous version so I can't confirm what date
really was. However, if the value was being updated every sync, it would mean that there was a new entry created in the destination every time. This will not be the case anymore. Is this an issue? Should we document this behavior change?
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.
You could find previous version here
This is an issue, because data will be returned for ALL period, see https://github.com/airbytehq/airbyte/pull/33930/files#diff-ade20df2f7ea130a82ed265a6a96eb70c0d9ccb98e92bb61afdb5b05c146525fR33
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.
So basically, we are moving from metric_type=time_series
to metric_type= total_value
, right? If so, is this what the customer wants? This seems like a different stream entirely
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.
Not really, we'll receive the most similar
metrics as it was described in FB docs (alternative metric)
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.
Yes but they are not over time right?
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.
No, they are "lifetime", just 1 record per metric without any 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.
Now yes, but before, "date" was part of the primary key right? So we could have multiple entries in the database for the same metric (depending on the 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.
Sync discussion summary: The metric does not support time_series
. So this is a breaking change and we don't have an easy way around it. We're fine with the change and will try to intercept user feedback if this is not good enough. In the meanwhile, @artem1205 will document the change better in the migration guide.
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!
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.
please add link to migration guide and make other changes requested
Co-authored-by: Maxime Carbonneau-Leclerc <3360483+maxi297@users.noreply.github.com>
Co-authored-by: Maxime Carbonneau-Leclerc <3360483+maxi297@users.noreply.github.com>
Co-authored-by: Maxime Carbonneau-Leclerc <3360483+maxi297@users.noreply.github.com>
What
Resolve #33899
How
upgrade to v18
Recommended reading order
airbyte-integrations/connectors/source-instagram/setup.py
streams.py
🚨 User Impact 🚨
Reauthentication for all users required, scope added:
business_management
see https://github.com/airbytehq/airbyte-platform-internal/pull/10582
Stream Media Insights
New metrics for Reels:
ig_reels_avg_watch_time
,ig_reels_video_view_total_time
Stream UserLifetimeInsights
Metric
audience_locale
become unavailable.Metrics
audience_city
,audience_country
andaudience_gender_age
changed to one metricfollower_demographics
with respective breakdownscity
,country
,age,gender
.primary key changed to ["business_account_id", "breakdown"]
date property deleted.
Metric type set to
total_value
, see docs for more info.Stream StoryInsights
deprecated metrics:
exits
,taps_forward
,taps_back
Pre-merge Actions
Updating a connector
Community member or Airbyter
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.