Sync all fields #18
Merged
Sync all fields #18
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
asaf-erlich
reviewed
Nov 12, 2020
| # we mirror that here | ||
| transformed_data = transform_json({data_key: list(raw_records.values())}, | ||
| stream_name)[data_key] | ||
| if not transformed_data: |
There was a problem hiding this comment.
Maybe check earlier if raw records is empty
asaf-erlich
reviewed
Nov 12, 2020
|
|
||
| return total_records, max_bookmark | ||
| def get_next_url(data): |
There was a problem hiding this comment.
Potentially reuse this method in their code too...
asaf-erlich
approved these changes
Nov 12, 2020
luandy64
added a commit
that referenced
this pull request
Nov 12, 2020
* Remove formatting a string * Add new function to date window the ad analytics sync * Use new sync function * Reduce the number of times we write the schema to once per selected stream * Update windowing logic to work for X days * Make date_window_size configurable * Make pylint happy * Update and clarify a comment * Sync all fields (#18) * Remove restriction on the number of fields you can select * Add a new function that mirrors the functionality of sync_endpoint * Add function to chunk fields, add unit test * Add unit tests for get_next_url * Add shift_sync_window, add unit test for shift_sync_window * Add merge_responses, add unit tests for merge_responses * WIP: requesting <20 fields works as expected * WIP: get bookmarks working * Make pylint happy * Clean up comments * Refactor start_date to last_datetime for clairity * Finish refactor * Fix date time parsing bug from refactor * Make pivot and pivot_value automatic for ad_analytics_by_campaign * Add optional param as a test seam * Add some tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of change
LinkedIn made a change to their API to limit the number of fields we can sync. This PR allows you to sync all fields for the
ad_analytics_by_campaignstream.The current implementation of
sync_endpoint()has 3 jobs:transform_json()process_records()This PR breaks
ad_analytics_by_campaignout of that flow, but tries to mirror it as closely as possible.The only change is to the "Get a page of records from the API" step. Depending on the number of fields selected, we make multiple requests to get the data and then merge these requests into the format that
transform_json()expects.Manual QA steps
Risks
Both syncs saw cost_in_local_currency but got different values 49.02123 | 49.02Both syncs saw approximate_unique_impressions but got different values 3639 | 3638Rollback steps