Connector Facebook-Marketing: update insights streams with custom entries for fields, breakdowns and action_breakdowns #4864

vladimir-remar · 2021-07-20T15:53:41Z

What

In our integration with the Facebook-Marketing connector when we trying to synchronize the API data from insights all attempts were unsuccessful (see logs on Notes). After experiencing the same type of behavior in Stitch, the solution is to request less fields to Facebook API. For thar reason, it is proposed is to add custom dynamic schemas to reduce the number of fields that are sent in each request. This means, users will be able to choose the fields that are requested and obtained in the response. After seeing the positive results both in Stitch and in a custom Facebook-Marketing connector created by us, we propose the following alternative.

How

The solution is inspired by the connector of Google-Analytics-Singer that adds "custom_reports" that are configured in the config. With this idea we propose the following:

Add to the config an optional field custom_insights_fields that contains the fields separated by commas that we want to send in each request to the insights api.
In the Streams file, create new classes (CustomAdsInsights, CustomAdsInsightsAgeAndGender, CustomAdsInsightsCountry, CustomAdsInsightsRegion, CustomAdsInsightsDma, CustomAdsInsightsPlatformAndDevice) with the prefix "Custom" to differentiate them from the rest and that these classes inherit the behavior of the master class AdsInsights and only varying the final resulting schema and fields in these new classes.
In the Source file modify the streams method so that it is updated if the new custom_insights_fields field is configured.

Notes

I'm adding some logs and some captures.
error.log

I acknowledge this first commit is missing the Documentation updates and the docker version bump.

Pre-merge Checklist

Expand the checklist which is relevant for this PR.

Connector checklist

Connector Generator checklist

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed.

sherifnada · 2021-07-21T01:05:49Z

@vladimir-remar would this be supplanted by the solution in #2227 ?

vladimir-remar · 2021-07-21T08:55:33Z

@sherifnada Yes, but, depending on the implementation, the Facebook-Marketing connector will need some additional changes as the request to the insights API has to explicitly include the limited number of fields.
Since we do not know when solution to #2227 will be implemented, does it make sense to merge the solution I propose in the PR? The only way to synchronize the Facebook-Marketing data with our destination, as of now, is to implement a custom Facebook connector.

vladimir-remar · 2021-07-22T10:14:26Z

@sherifnada any thoughts or suggestions?

marcosmarxm · 2021-08-24T17:34:15Z

@keu is possible to review this contribution?

vladimir-remar · 2021-09-20T09:52:22Z

@marcosmarxm Hi again.
After this time it makes sense to think of an approach more similar to the one proposed in this PR, as I said the only way we can synchronize the data from facebook is using the custom streams (at least for now).
I also take into account the #2227 but I don't know the status.

keu · 2021-09-20T14:25:47Z

@vladimir-remar Insights is already pretty custom streams. As I remember after my last refactor it should be easy to make it fully custom. The only reason why I didn't is that it breaks backward compatibility.

You just need to move all hardcoded attributes to config and build instances of the InsightsStream in streams method.
So the end result will replace all insights streams with a list of configuration objects in the config.

{
"insights":
 [
   {
    "name": "text",
    "fields": [],
    "breakdowns": [],
    "action_breakdowns": [],
   }
 ]
}

vladimir-remar · 2021-09-20T14:56:51Z

@vladimir-remar Insights is already pretty custom streams. As I remember after my last refactor it should be easy to make it fully custom. The only reason why I didn't is that it breaks backward compatibility.

You just need to move all hardcoded attributes to config and build instances of the InsightsStream in streams method.
So the end result will replace all insights streams with a list of configuration objects in the config.
{
"insights":
 [
   {
    "name": "text",
    "fields": [],
    "breakdowns": [],
    "action_breakdowns": [],
   }
 ]
}

@keu Ok, for each Insights stream we will have a different configuration and if some insights were not in the json object we would have to define default values for the streams that are not included in this object, right?

keu · 2021-09-21T12:42:52Z

@keu Ok, for each Insights stream we will have a different configuration and if some insights were not in the json object we would have to define default values for the streams that are not included in this object, right?

I see two options to keep existing insights streams:

define existing insights streams as default streams in the streams method. Don't know how to handle naming conflict, probably with the last guy wins strategy.
make a default value for an array of streams in the spec.json. Not sure if it is possible.

Plus come up with a good auto-naming strategy (breakdowns joined with _ probably)

vladimir-remar · 2021-09-22T11:15:51Z

@keu Hello, I did some changes in order to improve the new approach, can you take a look?

CLAassistant · 2021-09-27T17:32:36Z

All committers have signed the CLA.

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/source.py

keu

great job in general, just a few comments

marcosmarxm · 2021-09-30T17:44:34Z

@vladimir-remar awesome work! thanks so much for this contribution. Small change, could you solve the conflict in source.py?

@keu can you final review this contribution?

…eams

vladimir-remar · 2021-10-01T06:54:33Z

@vladimir-remar awesome work! thanks so much for this contribution. Small change, could you solve the conflict in source.py?

@marcosmarxm done!

keu

lgtm

keu · 2021-10-04T12:58:01Z

@vladimir-remar don't forget to run ./gradlew format and tests

…eams

…custom-streams

vladimir-remar · 2021-10-05T15:39:54Z

@vladimir-remar Thanks. No need to bump, I will do it myself, just please add CHANGELOG to the facebook-marketing.md file.

@keu done!

sherifnada

@vladimir-remar thanks for the contribution! I certainly understand the value of having something like this that works well. Some points I'd like to make:

I'm wary of introducing a completely custom insights stream because the Facebook API is very volatile. Specifically: it is difficult to predict which inputs of fields/breakdowns/action_breakdowns are going to cause the insights sync to succeed or fail. For all of our current insights streams, we've done meticulous testing to make sure those requests succeed consistently. So the issue I'm worried about is a user might see this feature, try to use it, get a failure, and then assume the FB connector is unreliable and file support requests with Airbyte when really the issue is from the FB side. The error messages provided by the FB API tend to be pretty opaque too and unhelpful, e.g: Job Failed without any extra information.

This is all to say that, I think all of this should be made very very clear in the connector's documentation (docs/integrations/sources/facebook-marketing.md), that this is a "use with caution" or "experimental" feature. We should add a section about these custom insights streams in the docs describing all the above caveats.

Additionally, it would be great to have a way to tell the user ahead of time if their particular selection will work or not. For example, right now if they choose an incorrect field name, they will only know that problem at sync time not connector setup time.

WDYT?

sherifnada · 2021-10-06T01:02:22Z

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/source.py

+        insights_custom_streams = list()
+
+        for insight in insights:
+            args["name"] = insight.name


is this mutating the same config over and over, so in the end all the streams have the same config? can we copy the args instead? e.g:

arg_copy = copy.deepcopy(args) arg_copy["fields"] = ...

@sherifnada the streams being created each time, so config never shared

sherifnada · 2021-10-06T01:08:34Z

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/source.py

@@ -126,3 +148,28 @@ def spec(self, *args, **kwargs) -> ConnectorSpecification:
                ),
            ),
        )
+
+    def _update_insights_streams(self, insights, args, streams) -> List[Type[Stream]]:


I suggest the following approach for these custom streams:

The "standard" streams offered by the connector are always available (if a user doesn't want them they can always just deselect them)

If the user inputs any custom streams, they are named as custom_<user-input-name> and appended to the list of streams

This way it is very very obvious to the user what is happening. This is especially important as the connector's config is updated over time e.g: a user might call a stream ads_insights today, then remove it next week, and now the data represented in that stream is mixed with the "standard" ads insight stream and the "custom" insight stream. Making the suggestion above to make such situations much less likely.

I added the change in the lastest commit

…tation and fix to resolve in spec schema

vladimir-remar · 2021-10-07T11:05:12Z

To @keu thank you for help me in the spec limitation, was very helpful.
To @sherifnada I added some changes.

A way to verify the insights before the sync, in the check part.
An update to the documentation, warning the use for users.

vladimir-remar · 2021-10-07T11:07:34Z

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/source.py

+        return True
+
+
+def expand_local_ref(schema, resolver=None, **kwargs):


@sherifnada thanks to @keu
With this, we can set properly the insights in the UI.

vladimir-remar · 2021-10-13T10:00:52Z

@sherifnada after the latest changes, the user will only be allowed to configure the fields provided by Facebook, but the combination of these will be left to the user's choice.
WDYT?

sherifnada

@vladimir-remar thanks for adding the validations! just a couple of comments and I think we are good to go

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/source.py

sherifnada · 2021-10-13T22:55:18Z

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/source.py

@@ -26,6 +35,17 @@
 )


+class InsightConfig(BaseModel):


@vladimir-remar did you validate that the json output by spec works with the UI via the instructions here? Just checking because I don't remember if we support a list of objects

@sherifnada we do actually :)

@sherifnada thanks to @keu, I attached some images from UI

It will be filled like this

And it will look like this with two elements

and from destination side

…ger import

sherifnada

LGTM, thanks @vladimir-remar !

@keu could you assign to someone on the team for publishing?

yevhenii-ldv · 2021-10-19T12:09:22Z

Hi @vladimir-remar, your changes was merged and published in PR #7158

zestyping · 2021-12-01T20:31:30Z

This is awesome!! Thank you so much!

Connector Facebook-Marketing: update streams with custom streams

1fa4461

github-actions bot added the area/connectors Connector related issues label Jul 20, 2021

sherifnada added the community_pr label Jul 20, 2021

marcosmarxm requested a review from keu August 24, 2021 17:33

marcosmarxm self-assigned this Aug 27, 2021

update: remove custom streams, add new custom insights from config

4cf644e

keu reviewed Sep 29, 2021

View reviewed changes

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/source.py Outdated Show resolved Hide resolved

keu reviewed Sep 29, 2021

View reviewed changes

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/source.py Outdated Show resolved Hide resolved

keu reviewed Sep 29, 2021

View reviewed changes

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/source.py Outdated Show resolved Hide resolved

keu suggested changes Sep 29, 2021

View reviewed changes

update: add new model for InsightConfig, remove old imports

ab6fc13

vladimir-remar marked this pull request as ready for review September 29, 2021 11:13

vladimir-remar changed the title ~~Connector Facebook-Marketing: update streams with custom streams~~ Connector Facebook-Marketing: update insights streams with custom entries for fields, breakdowns and action_breakdowns Sep 29, 2021

vladimir-remar requested a review from keu September 29, 2021 11:14

Merge branch 'master' into vladimir/connector-facebook-add-custom-str…

2601654

…eams

keu approved these changes Oct 4, 2021

View reviewed changes

Merge branch 'master' into vladimir/connector-facebook-add-custom-str…

b9dfed8

…eams

vladimir-remar added 2 commits October 5, 2021 17:35

Merge branch 'airbytehq:master' into vladimir/connector-facebook-add-…

892279f

…custom-streams

update Changelog

528cd44

keu requested a review from sherifnada October 5, 2021 15:43

github-actions bot added the area/documentation Improvements or additions to documentation label Oct 5, 2021

sherifnada reviewed Oct 6, 2021

View reviewed changes

update: add to check a validation to insights entries, update documen…

b47be69

…tation and fix to resolve in spec schema

vladimir-remar commented Oct 7, 2021

View reviewed changes

merge master

8c0f243

vladimir-remar requested a review from sherifnada October 7, 2021 11:21

merge origin

8dcf971

sherifnada reviewed Oct 13, 2021

View reviewed changes

marcosmarxm added community and removed community_pr labels Oct 14, 2021

vladimir-remar added 2 commits October 14, 2021 11:48

fix: fix import logger from entrypoint, change to python logger

20df1e0

fix: change error message from check custom insights entries, fix log…

dd00338

…ger import

vladimir-remar requested a review from sherifnada October 14, 2021 09:52

sherifnada approved these changes Oct 15, 2021

View reviewed changes

yevhenii-ldv mentioned this pull request Oct 19, 2021

🎉 Connector Facebook-Marketing: update insights streams with custom entries for fields, breakdowns and action_breakdowns #7158

Merged

38 tasks

yevhenii-ldv temporarily deployed to more-secrets October 19, 2021 11:13 Inactive

jrhizor temporarily deployed to more-secrets October 19, 2021 11:15 Inactive

yevhenii-ldv closed this Oct 19, 2021

karinakuz added connectors/source/facebook-marketing connectors/source/apify-dataset connectors/sources-api and removed connectors/source/apify-dataset labels Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connector Facebook-Marketing: update insights streams with custom entries for fields, breakdowns and action_breakdowns #4864

Connector Facebook-Marketing: update insights streams with custom entries for fields, breakdowns and action_breakdowns #4864

vladimir-remar commented Jul 20, 2021

sherifnada commented Jul 21, 2021

vladimir-remar commented Jul 21, 2021

vladimir-remar commented Jul 22, 2021

marcosmarxm commented Aug 24, 2021

vladimir-remar commented Sep 20, 2021 •

edited

Loading

keu commented Sep 20, 2021 •

edited

Loading

vladimir-remar commented Sep 20, 2021 •

edited

Loading

keu commented Sep 21, 2021 •

edited

Loading

vladimir-remar commented Sep 22, 2021

CLAassistant commented Sep 27, 2021 •

edited

Loading

keu left a comment

marcosmarxm commented Sep 30, 2021

vladimir-remar commented Oct 1, 2021

keu left a comment

keu commented Oct 4, 2021 •

edited

Loading

vladimir-remar commented Oct 5, 2021

sherifnada left a comment

sherifnada Oct 6, 2021

keu Oct 6, 2021

sherifnada Oct 6, 2021

vladimir-remar Oct 7, 2021

vladimir-remar commented Oct 7, 2021

vladimir-remar Oct 7, 2021

vladimir-remar commented Oct 13, 2021

sherifnada left a comment

sherifnada Oct 13, 2021

keu Oct 14, 2021

vladimir-remar Oct 14, 2021

sherifnada left a comment

yevhenii-ldv commented Oct 19, 2021

zestyping commented Dec 1, 2021

		return True


		def expand_local_ref(schema, resolver=None, **kwargs):

Connector Facebook-Marketing: update insights streams with custom entries for fields, breakdowns and action_breakdowns #4864

Connector Facebook-Marketing: update insights streams with custom entries for fields, breakdowns and action_breakdowns #4864

Conversation

vladimir-remar commented Jul 20, 2021

What

How

Notes

Pre-merge Checklist

sherifnada commented Jul 21, 2021

vladimir-remar commented Jul 21, 2021

vladimir-remar commented Jul 22, 2021

marcosmarxm commented Aug 24, 2021

vladimir-remar commented Sep 20, 2021 • edited Loading

keu commented Sep 20, 2021 • edited Loading

vladimir-remar commented Sep 20, 2021 • edited Loading

keu commented Sep 21, 2021 • edited Loading

vladimir-remar commented Sep 22, 2021

CLAassistant commented Sep 27, 2021 • edited Loading

keu left a comment

Choose a reason for hiding this comment

marcosmarxm commented Sep 30, 2021

vladimir-remar commented Oct 1, 2021

keu left a comment

Choose a reason for hiding this comment

keu commented Oct 4, 2021 • edited Loading

vladimir-remar commented Oct 5, 2021

sherifnada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimir-remar commented Oct 7, 2021

Choose a reason for hiding this comment

vladimir-remar commented Oct 13, 2021

sherifnada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherifnada left a comment

Choose a reason for hiding this comment

yevhenii-ldv commented Oct 19, 2021

zestyping commented Dec 1, 2021

vladimir-remar commented Sep 20, 2021 •

edited

Loading

keu commented Sep 20, 2021 •

edited

Loading

vladimir-remar commented Sep 20, 2021 •

edited

Loading

keu commented Sep 21, 2021 •

edited

Loading

CLAassistant commented Sep 27, 2021 •

edited

Loading

keu commented Oct 4, 2021 •

edited

Loading