✨ Source Facebook Marketing: Add support for multiple Account IDs #33538

tolik0 · 2023-12-15T17:46:06Z

What

This PR introduces the capability to specify multiple Account IDs. Resolves: https://github.com/airbytehq/oncall/issues/3723

How

Modified the stream_slices method to iterate over a list of account_ids. Each account_id is explicitly passed to all relevant service classes, ensuring the correct account is used in all underlying processes.

🚨 User Impact 🚨

This PR introduces a backwards-incompatible change due to an alteration in the format of stream state. The format has been changed from:

{
  "stream_state": {
    "event_time": "2023-11-15T13:34:26Z",
    "include_deleted": true
  }
}

To:

"stream_state": {
    "0123456789": {
        "event_time": "2023-10-31T04:13:03+00:00",
        "include_deleted": false
    },
    "include_deleted": false
}

Now, all states are nested under the account_id.

vercel · 2023-12-15T17:46:13Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Jan 10, 2024 4:30pm

github-actions · 2023-12-15T17:46:28Z

Before Merging a Connector Pull Request

Wow! 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:

PR name follows PR naming conventions
Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
Secrets in the connector's spec are annotated with airbyte_secret
All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

airbyte-integrations/connectors/source-facebook-marketing/integration_tests/spec.json

artem1205 · 2023-12-21T10:37:54Z

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

@@ -76,6 +76,7 @@ def _validate_and_transform(self, config: Mapping[str, Any]):

        if config.end_date:
            config.end_date = pendulum.instance(config.end_date)
+


Should we add a validation for account_ids ?
Check for uniqueness, length, etc?

Fixed, regarding the length, Facebook doesn't provide any constraint.

github-actions · 2023-12-21T18:01:36Z

Warning

🚨 Connector code freeze is in effect until 2024-01-02. This PR is changing connector code. Please contact the current OC engineers if you want to merge this change to master.

erohmensing

I don't know all the intricacies and haven't dug too hard, but this looks straightforward and makes sense to me. Left some smaller comments, nothing big.

With the state transformation, I want to note that while this is not a breaking change in the sense that the customer will have to do something, it is breaking in the sense that in case something goes wrong, we cannot rollback. I think that's okay and worth avoiding a breaking change, but something to keep in mind should any OC issues with this connector occur. Ideally we have a better way of distinguishing these cases from one another in a more accessible way.

With the config transformation, the config control message is very similar - it allows us to migrate them forward but not roll back. I'm not sure how this applies to API users and terraform users - they will have their config updated in the DB, but it won't get back to their resources - so they will probably remain using the old format and also unaware that they can supply multiple, and the migration would run before every sync. I think we also manually update the config formats in the terraform provider - the point at which we did that would be a breaking change on the terraform provider side, I think? A bit out of my wheelhouse - @terencecho I'd love to know if y'all are thinking about stuff like this

airbyte-integrations/connectors/source-facebook-marketing/integration_tests/spec.json

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

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

...tions/connectors/source-facebook-marketing/source_facebook_marketing/streams/base_streams.py

airbyte-integrations/connectors/source-facebook-marketing/integration_tests/spec.json

erohmensing · 2023-12-21T22:10:02Z

...grations/connectors/source-facebook-marketing/source_facebook_marketing/config_migrations.py

+class MigrateAccountIdToArray:
+    """
+    This class stands for migrating the config at runtime,
+    while providing the backward compatibility when falling back to the previous source version.


This sentence is confusing to me - I think it provides compatibility to read data from the previous source version, but doesn't provide compatibility if the connector were to go back to the previous source version (after emitting state from the new version)

It meant that migration is backwards compatible as a new property is created instead of changing the existing one. If the user reverts to the previous version, the config will use the old account_id. I updated the docs to make sure everything is clear.

I see. This is related to my comment here - I didn't realize we didn't remove the old account_id field upon adding the new field

erohmensing · 2023-12-21T22:22:57Z

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

@@ -105,17 +105,15 @@ class ConnectorConfig(BaseConfig):
    class Config:


I assume this is generated, but my other comments to the spec.json apply here. Not sure which is generated from which

erohmensing · 2023-12-21T22:24:16Z

airbyte-integrations/connectors/source-facebook-marketing/integration_tests/conftest.py

+        config = json.load(config_file)
+        migrated_config = MigrateAccountIdToArray.transform(config)
+        return migrated_config


Is this necessary? I thought the source would perform the migration wherever we pass the config fixture into it

Migration is in the main.py, not the source:
https://github.com/airbytehq/airbyte/pull/33538/files#diff-1f20186eea3ef91ef3ce1053eb1dc02fd8ead91b098524ab68af68ae1e15f445R13-R15

If this was a conftest for unit tests this would make sense to me, but this is for cat tests which spin up a source container and run it - wouldn't that run main.py?

This fixture is passed directly to the source object without the main.py. https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connectors/source-facebook-marketing/integration_tests/test_streams.py#L29

erohmensing · 2023-12-21T22:27:34Z

...rations/connectors/source-facebook-marketing/unit_tests/test_migrations/test_new_config.json

+      "action_breakdowns": []
+    }
+  ],
+  "account_id": "01234567890",


the new config should not have this still, right?

also, nit: I would name this file test_config and the other one test_old_config - it is clearer migration wise and in future versions it is clear what is the current state of the config

Yes, account_id would be in the config if it is migrated; otherwise, only account_ids.

got it. in this case i think 3 test files could be here - one with account_id (previous version), one with account_id and account ids (upgraded from previous version), one with account_ids only (created or reset on new version)

I added one more testing config.

terencecho · 2023-12-27T17:19:30Z

@erohmensing unfortunately right now the terraform provider does not correctly detect configuration drift. So we would probably want to rely on outreach by our support team.

In the future when we do get the drift detection working, users will notice the change in their resource the next time they run terraform plan or apply and it will be their responsibility to double check, resolve the changes by updating their tf code.

midavadim · 2024-01-09T22:53:19Z

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

+            for account_id in config.account_ids:
+                # Get Ad Account to check creds
+                ad_account = api.get_account(account_id=account_id)
+                logger.info(f"Select account {ad_account}")


move logger before get_account, becuase it run actual reguest using specified accunt , so from logs we'll know which account is failing

midavadim · 2024-01-09T22:56:28Z

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

-            # Get Ad Account to check creds
-            ad_account = api.account
-            logger.info(f"Select account {ad_account}")
+        except facebook_business.exceptions.FacebookRequestError as e:


overall about error handling and logging - we have to be sure that log messages contain info about failing account id

Overall - we need to decide whether we should stop the whole sync if an error happens for some specific account

The error should be raised during the check to indicate missing access to one of the accounts. Additionally, during the sync, the error will be raised to avoid silently missing part of the data. Such errors will require the user to fix the connection configuration before proceeding, thus ensuring no data is missed.

…rbytehq#33538)

octavia-squidington-iii added area/connectors Connector related issues connectors/source/facebook-marketing labels Dec 15, 2023

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Dec 15, 2023

tolik0 self-assigned this Dec 15, 2023

tolik0 requested review from midavadim and a team December 15, 2023 17:47

tolik0 marked this pull request as ready for review December 15, 2023 17:47

vercel bot deployed to Preview December 15, 2023 17:50 View deployment

tolik0 requested a review from bazarnov December 15, 2023 18:15

artem1205 reviewed Dec 18, 2023

View reviewed changes

airbyte-integrations/connectors/source-facebook-marketing/integration_tests/spec.json Outdated Show resolved Hide resolved

octavia-squidington-iv requested a review from a team December 18, 2023 19:31

tolik0 force-pushed the tolik0/source-facebook-marketing/add-multiple-account-ids branch from 646f002 to 6a57d60 Compare December 20, 2023 19:59

tolik0 requested a review from artem1205 December 20, 2023 19:59

artem1205 requested changes Dec 21, 2023

View reviewed changes

octavia-squidington-iv requested a review from a team December 21, 2023 10:39

tolik0 force-pushed the tolik0/source-facebook-marketing/add-multiple-account-ids branch from ce51f54 to 6e8e89b Compare December 21, 2023 18:01

tolik0 requested review from artem1205 and erohmensing December 21, 2023 21:05

erohmensing approved these changes Dec 21, 2023

View reviewed changes

tolik0 requested a review from terencecho December 27, 2023 13:06

vercel bot deployed to Preview January 3, 2024 12:44 View deployment

tolik0 and others added 5 commits January 9, 2024 15:28

Add possibility to use multiple Account IDs

cff2aac

Update version

b0e0fd7

Delete debug logs

4f05f6f

Add migration to new property account_ids

904f7e1

formatted

fdaf9cd

tolik0 added 4 commits January 9, 2024 15:31

Fix validation of Account ID

347c96f

Fix comments from review

f8e0110

Add one more config file for the migration tests

f78a648

Move migration to run.py

99fc2d1

tolik0 force-pushed the tolik0/source-facebook-marketing/add-multiple-account-ids branch from e9354f1 to 99fc2d1 Compare January 9, 2024 15:44

vercel bot deployed to Preview January 9, 2024 17:21 View deployment

Fix format

5149197

tolik0 force-pushed the tolik0/source-facebook-marketing/add-multiple-account-ids branch from 1108736 to 5149197 Compare January 9, 2024 18:38

artem1205 approved these changes Jan 9, 2024

View reviewed changes

terencecho removed their request for review January 9, 2024 19:44

midavadim reviewed Jan 9, 2024

View reviewed changes

Add logging

00eb297

tolik0 requested a review from midavadim January 10, 2024 12:30

Fix unit test

bcabe8a

tolik0 merged commit 3be28af into master Jan 10, 2024
24 checks passed

tolik0 deleted the tolik0/source-facebook-marketing/add-multiple-account-ids branch January 10, 2024 21:12

marcosmarxm mentioned this pull request Feb 13, 2024

Source Facebook Marketing: Support multiple Facebook accounts #7740

Closed

jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024

✨ Source Facebook Marketing: Add support for multiple Account IDs (ai…

cda0fb0

…rbytehq#33538)

jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024

✨ Source Facebook Marketing: Add support for multiple Account IDs (ai…

383dcdd

…rbytehq#33538)

jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024

✨ Source Facebook Marketing: Add support for multiple Account IDs (ai…

3222f9d

…rbytehq#33538)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Source Facebook Marketing: Add support for multiple Account IDs #33538

✨ Source Facebook Marketing: Add support for multiple Account IDs #33538

tolik0 commented Dec 15, 2023 •

edited

Loading

vercel bot commented Dec 15, 2023 •

edited

Loading

github-actions bot commented Dec 15, 2023 •

edited by tolik0

Loading

artem1205 Dec 21, 2023

tolik0 Dec 21, 2023 •

edited

Loading

github-actions bot commented Dec 21, 2023 •

edited

Loading

erohmensing left a comment

erohmensing Dec 21, 2023

tolik0 Dec 22, 2023 •

edited

Loading

erohmensing Dec 22, 2023

erohmensing Dec 21, 2023

erohmensing Dec 21, 2023

tolik0 Dec 22, 2023

erohmensing Dec 22, 2023 •

edited

Loading

tolik0 Dec 28, 2023

erohmensing Dec 21, 2023

erohmensing Dec 21, 2023

tolik0 Dec 22, 2023

erohmensing Dec 22, 2023

tolik0 Dec 28, 2023

terencecho commented Dec 27, 2023

midavadim Jan 9, 2024

tolik0 Jan 10, 2024

midavadim Jan 9, 2024

midavadim Jan 9, 2024

tolik0 Jan 9, 2024

		@@ -76,6 +76,7 @@ def _validate_and_transform(self, config: Mapping[str, Any]):

		if config.end_date:
		config.end_date = pendulum.instance(config.end_date)

		@@ -105,17 +105,15 @@ class ConnectorConfig(BaseConfig):
		class Config:

✨ Source Facebook Marketing: Add support for multiple Account IDs #33538

✨ Source Facebook Marketing: Add support for multiple Account IDs #33538

Conversation

tolik0 commented Dec 15, 2023 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

vercel bot commented Dec 15, 2023 • edited Loading

github-actions bot commented Dec 15, 2023 • edited by tolik0 Loading

Before Merging a Connector Pull Request

Choose a reason for hiding this comment

tolik0 Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 21, 2023 • edited Loading

erohmensing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolik0 Dec 22, 2023 • edited Loading

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

erohmensing Dec 22, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencecho commented Dec 27, 2023

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

tolik0 commented Dec 15, 2023 •

edited

Loading

vercel bot commented Dec 15, 2023 •

edited

Loading

github-actions bot commented Dec 15, 2023 •

edited by tolik0

Loading

tolik0 Dec 21, 2023 •

edited

Loading

github-actions bot commented Dec 21, 2023 •

edited

Loading

tolik0 Dec 22, 2023 •

edited

Loading

erohmensing Dec 22, 2023 •

edited

Loading