Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[TDL-24367] Fix streams implementation #64

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

[TDL-24367] Fix streams implementation #64

wants to merge 24 commits into from

Conversation

shantanu73
Copy link

@shantanu73 shantanu73 commented Oct 25, 2023 •
edited
Loading

Description of change

JIRA

  1. Upgraded LinkedIn API version from 202302 to 202309
  2. Schema changes for video_ads stream
  3. Modified path for streams campaign_groups, campaigns & creatives to include /adAccounts/
  4. Added logic to iterate over each accountID in config.json for above mentioned streams
  5. Changed base path from "adDirectSponsoredContents" to "posts" for video_ads stream due to API upgrade
  6. Changed primary key from "content_reference" to "id" for video_ads stream due to API upgrade
  7. Changed replication key from "last_modified_time" to "last_modified_at" for video_ads stream due to API upgrade
  8. Fixed search & sort parameters for account_users & campaigns streams.
  9. Removed the account filter "search_account_values_param".
  10. Added config param for sync_endpoint method.
  11. Added transform logic for AdContext fields.
  12. Removed pivot & pivotValue fields and added pivotValues field for Ad Analytics streams.

Manual QA steps

  • Ran discovery & sync in local dev environment
  • Generated target csv files to validate replicated data

Risks

  • API upgrade changes can cause video_ads, ad_analytics_by_creative, ad_analytics_by_campaign streams to fail

Rollback steps

  • revert this branch

@shantanu73
Update LinkedIn API version from 202302 to 202308
937ec47
@shantanu73
Changes:
84ce4dc 
1) Changed LinkedIn API version to 202309.
2) Removed logic to add account parameter for streams with account filter search_account_param.
@shantanu73
Changes:
661f1ee 
1) Added logic to iterate over each account and use modified url for campaigns, campaigngroups & creatives APIs.
2) Added X-Restli-Protocol-Version header & modified params fir campaigns & campaigngroups APIs.
3
) Removed account_filter for campaigns & campaigngroups APIs.
4) Added a new parameter config for sync_endpoint method.
@shantanu73
Added relevant search query fields for analytics streams.
aa1ec85
@shantanu73
Changes:
4c0c8bd 
1) Added new schema for video_ads stream.
2) Modified enpoint and query params for video_ads stream.
3) Removed transform logic to flatten video_ads schema for change_audit_stamps fields.
@shantanu73
Changes:
393dfb3 
1) Modified schema for video_ads stream.
2) Added transform logic to add ad_context fields to root level.
@shantanu73
Modified logic to fetch pivot value from the list.
13e9490
@shantanu73
Fixed pylint issues.
ea6e61d
@shantanu73
Fixed pylint for duplicate code.
e19aa0e
@shantanu73
Fixed streams, sync & transform unit tests.
57276b1
@shantanu73
Added back the logic to tranform audit fields.
cf70819
@shantanu73
Removed logic to delete Beta fields from Ad analytics streams.
f57f63d
somethingmorerelevant
somethingmorerelevant requested changes Nov 16, 2023
View reviewed changes
tap_linkedin_ads/streams.py Outdated
Comment on lines 317 to 319
if self.tap_stream_id in NEW_PATH_STREAMS:
querystring = '&'.join(['%s=%s' % (key, value) for (key, value) in endpoint_params.items()])
account_list = config['accounts'].replace(" ", "").split(",")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is adds a child stream dependancy in the parent class, sync_endpoint method should be modified in the child class.

@shantanu73
Changes:
6bd4e7a 
1) Added get_config() method in LinkedInClient class to fetch config json.
2) Updated usage of get_config() method in discover & sync for tap.
@shantanu73
Added usage for get_config() method in sync_endpoint() method.
afb2d2f
@shantanu73
Added config instance variable to LinkedInClient class and updated it…
59c5266 
…'s usage.
@shantanu73
Fixed test_sync unit test as per new config paramter changes.
a00f213
@shantanu73
Removed get_config method and added config instance variable in Linke…
f3efe12 
…dinClient class and updated its usage.
@shantanu73
Fixed test_main & test_sync unit tests as per new config changes.
d21583e
@shantanu73
Changed the order of config parameter in LinkedinClient object.
754e11f
@shantanu73
Changes:
cfee98e 
1) Fixed unit tests as per the new config param changes in LinkedinClient class.
2) Made changes in check_accounts() method as per config param changes.
3) Fixed unit tests as per changes in check_accounts() method.
@shantanu73
Changes:
afa8785 
1) Fixed dangerous default value pylint error in LinkedinClient class for config parameter.
2) Fixed unit tests as per above change.
@shantanu73
[TDL-24282] Fix integration tests (#65)
6d2cc56 
* Changes:
1. Fixed video_ads metadata.
2. Fixed all fields & automatic fields tests.

* Removed analytics streams from start_date, automatic_fields & bookmarks tests.

* Fixed Parent child independent test for Ad analytics streams.

* Removed unnecessary fields.

* Removed beta fields from KNOWN_MISSING_FIELDS in All fields test for ad analytics streams.

* Changes:
1) Added comment mentioning why the Ad Analytics streams were removed from testing.
2) Removed unneccary whitespace.

* Fixed formatting.
@shantanu73
Updated scope information for video_ads stream in new API version upg…
80bcf5e 
…rade in README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@somethingmorerelevant somethingmorerelevant somethingmorerelevant approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shantanu73 @somethingmorerelevant