🚨🚨Source Facebook Marketing: Updated breakdowns schema - #36608
Daryna Ishchenko (darynaishchenko) merged 21 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Octavia Squidington III (octavia-squidington-iii)
added
area/connectors
Daryna Ishchenko (darynaishchenko)
changed the title
…reakdown-properies
…reakdown-properies
…reakdown-properies
…reakdown-properies
…reakdown-properies
Maxime Carbonneau-Leclerc (maxi297)
left a comment
Maxime Carbonneau-Leclerc (maxi297)
left a comment
There was a problem hiding this comment.
This change overall makes sense to me. I'd like to address my two comments before approving
| - "ads_insights_dma" | ||
| - "ads_insights_action_product_id" | ||
| 3.0.0: | ||
| message: "All Custom Insights Streams that uses body_asset, call_to_action_asset, description_asset, image_asset, link_url_asset, title_asset or video_asset as breakdown now have updated schemas. Users will need to retest source configuration, refresh the source schema and reset affected streams after upgrading. " |
There was a problem hiding this comment.
It's a shame that we can't target streams more specifically here as everyone will have to opt-in instead of just those with this specific situation.
Ella Rohm-Ensing (@erohmensing) do you know if there is a way to scope that better? Basically, the user can enter custom_insights.X.name which will create a stream named custom<custom_insights.X.name>. If we can scope even more/in another way, targeting some fields from the json schema would also be good.
There was a problem hiding this comment.
if I user does not use body_asset, call_to_action_asset, description_asset, image_asset, link_url_asset, title_asset or video_asset they dont need to reset?
There was a problem hiding this comment.
Kat Wilson (@katmarkham), you don't need a reset as your stream schema doesn't have fields above. The source returns stream schema according to provided breakdowns in the custom stream config.
There was a problem hiding this comment.
Maybe a broader discussion for my personal learning: do we need to make it a breaking change if we have these options now?
If so, I would keep the opt-in mechanism only for when the config is changing. In our case, it is only a schema change
There was a problem hiding this comment.
As Daryna Ishchenko (@darynaishchenko) mentioned in another comment, this would still be a breaking change because of the PK changing
There was a problem hiding this comment.
Ella Rohm-Ensing (@erohmensing) do you know if there is a way to scope that better? Basically, the user can enter custom_insights.X.name which will create a stream named custom<custom_insights.X.name>. If we can scope even more/in another way, targeting some fields from the json schema would also be good.
I know this is over, but wanted to add - unfortunately I looked into this when we were creating it, but making some sort of regex matching or globbing proved to be something we couldn't implement because we are very lenient with characters in the stream names, meaning any character like * could be a wildcard or an actual character in the stream name.
| }, | ||
| "body_asset": { | ||
| "description": "The main content or message of the ad.", | ||
| "type": ["null", "object"], |
There was a problem hiding this comment.
One more random thought: we could maybe avoid the breaking change by adding a new field instead of re-using the same one. Something like body_asset_object and we can keep body_asset as str(data["body_asset"]). What I don't like about this solution though is that we start to drift from the API naming and it adds complexity in our code.
There was a problem hiding this comment.
It would be a still breaking change due to change in pk. Because we also changed pk from value of str(data["body_asset"]) to body_asset_id.
| - "ads_insights_dma" | ||
| - "ads_insights_action_product_id" | ||
| 3.0.0: | ||
| message: "All Custom Insights Streams that uses body_asset, call_to_action_asset, description_asset, image_asset, link_url_asset, title_asset or video_asset as breakdown now have updated schemas. Users will need to retest source configuration, refresh the source schema and reset affected streams after upgrading. " |
There was a problem hiding this comment.
if I user does not use body_asset, call_to_action_asset, description_asset, image_asset, link_url_asset, title_asset or video_asset they dont need to reset?
| - "ads_insights_dma" | ||
| - "ads_insights_action_product_id" | ||
| 3.0.0: | ||
| message: "All Custom Insights Streams that uses body_asset, call_to_action_asset, description_asset, image_asset, link_url_asset, title_asset or video_asset as breakdown now have updated schemas. Users will need to retest source configuration, refresh the source schema and reset affected streams after upgrading. " |
There was a problem hiding this comment.
| message: "All Custom Insights Streams that uses body_asset, call_to_action_asset, description_asset, image_asset, link_url_asset, title_asset or video_asset as breakdown now have updated schemas. Users will need to retest source configuration, refresh the source schema and reset affected streams after upgrading. " | |
| message: "There are breaking schema changes to Custom Insights Streams that use body_asset, call_to_action_asset, description_asset, image_asset, link_url_asset, title_asset or video_asset. If you use any of these streams, you will need to retest source configuration, refresh the source schema and reset affected streams after upgrading. " |
There was a problem hiding this comment.
updated
Maxime Carbonneau-Leclerc (maxi297)
left a comment
Maxime Carbonneau-Leclerc (maxi297)
left a comment
There was a problem hiding this comment.
I'm good with this change. Thanks for addressing my questions/concerns!
…reakdown-properies
This branch was successfully deployed
6 participants
OC issue: https://github.com/airbytehq/oncall/issues/4527
Schema for breakdowns fields was updated based based on user data and docs from string to object.
🚨Breaking changes: This PR contains breaking changes. Users will need to refresh schema and reset affected stream.
Affected stream: only custom insights reports as schema for AdInsights streams created according to defined breakdowns and base streams don't use breakdowns that were changed.