TDL-9902 removed the automatic fields from ad ext report #85
Conversation
dbshah1212
changed the base branch from
master
to
TDL-10048-Improve-BingAds-Reliability
…b.com/singer-io/tap-bing-ads into TDL-9902-update-auto-fields-for-ext-details
tap_bing_ads/reports.py
Outdated
| 'Clicks', | ||
| 'Ctr', | ||
| 'Impressions' | ||
| 'AdExtensionTypeId' |
There was a problem hiding this comment.
@namrata270998 Can you please add a comment regarding the change you have made in this PR.
Also add a code comment above this line regarding what these fields are meant for
There was a problem hiding this comment.
Bing has a requirement that each report must have at least 1 performance based metric (Clicks, Impressions, etc.) selected to generate a report. We force Impressions, Ctr,, and Clicks to be automatic for the ad_extension_detail_report. This is inconsistent with how we treat field selection for other reports as we do not impose any performance measures on any other report streams. It should be up to user, which perf measures should be selected. Hence, removed the Impressions, Ctr,, and Clicks from required fields.
|
@umangagrawal-crest @namrata270998 As discussed with Krishnan, please add a comment in the JIRA regarding what needs to be added to the documentation or the behavioral change incorporated as part of this PR |
There was a problem hiding this comment.
While the base expectations were properly updated, we are not actually testing this feature. The assertions are commented out in the discovery test, because the tap does not conform to the metadata standards. I believe this PR must incorporate the changes from #91
|
@kspeer825 Sure will merge this PR after the #91 gets merged. |
There was a problem hiding this comment.
I can approve this when the dependencies are brought in from #91. I am not able to keep up with the changes across PRs to reconcile the differences. Ya'll are too fast for me with these PRs. In the future we will try to organize the Jira tickets so that there is a clear order of operations and one ticket will not unexpectedly block another after the fact.
…b.com/singer-io/tap-bing-ads into TDL-9902-update-auto-fields-for-ext-details
…b.com/singer-io/tap-bing-ads into TDL-9902-update-auto-fields-for-ext-details
@kspeer825 we merged the code of PR 91 here and the build is passing, can you please review it now? |
…b.com/singer-io/tap-bing-ads into TDL-9902-update-auto-fields-for-ext-details
…/github.com/singer-io/tap-bing-ads into TDL-9902-update-auto-fields-for-ext-details
* TDL-16888 Added code comments (#88) * Added code comments * Updated code comments * UPdated comment * TDL 7402 Add retry logic for 500 errors (#90) * Initial commit for backoff error handling * Added usage of error handling function * Added detail comment for unittest cases * Added coverage report to artifact * added backoff for Expection(408) * Updated backoff for async method * updated backoof for poll_report * Updated max_time * Updated unittest cases * removed Exception from backoff * Updated backoff * resolved unittest case errro * Updated typo in comments * Added test cases for Exception with error code 408 * Added comment for 408 error * Added comments * Updated unit test cases for 400 error * Updated unit test case for create sdk client * Merged elif condition * Updated unit test cases * Tdl 9721 missing top level breadcrumb for all streams (#91) * TDL-9721: Updated metadata creation method and added top level breadcrum and un commented the breadcrum test case code. * TDL-9721: Added top level breadcrumb. * TDL-9721: Commented the code wich uncommented by mistake and failing tests * TDL-9721: Removed unused changes * TDL-9721: Updated automatic fields for report streams * TDL-9721: Optimized the code of making replication key automatic * TDL-9721: Added Unit tests with coverage * TDL-9721: Added pip install nose * TDL-9721: Updated file name * TDL-9721: Updated config.yml * TDL-9721: Added missing fieldExclusion * TDL-9721: Added code comment * TDL-9721: Updated the primary key of account stream as earlier. * TDL-9721: Updated primary key for account stream in tests to make it same as catalog. * TDL-9721: Updated primary key in catalog for account stream. * TDL-9721: Updated the replication keys and test cases. * TDL-9721: Updated replication key for discover mode only as other test using replication key. * TDL-6737 added pylint and resolved the errors (#87) * added pylint and resolved the errors * added pylint in the setup.py * corrected the pylint command * added package name * replaced dev with test in pip install * resolved the comments * removed f-string * Added covergae report to artifact * removed unittest case report * removed config file changes * revert back config.yml file changes * Added back pylint in circleci * resolved pylint error * resolved comments * removed str * removed logger statements * added consider-using-f-string in config.yml * resolved comments * resolved pylint errors * pylint fixtures Co-authored-by: prijendev <prijen.khokhani@crestdatasys.com> * TDL-15251: Add required fields for age_gender_audience_report (#86) * added required fields for age_gender_audience_report * Added coverage report to artifact * revert back coverage report changes * revert back config.yml file changes * resolved comments * added missing assertions * resolved cci failures Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> * Tdl 7402 add retry logic for 500 errors (#96) * Initial commit for backoff error handling * Added usage of error handling function * Added detail comment for unittest cases * Added coverage report to artifact * added backoff for Expection(408) * Updated backoff for async method * updated backoof for poll_report * Updated max_time * Updated unittest cases * removed Exception from backoff * Updated backoff * resolved unittest case errro * Updated typo in comments * Added test cases for Exception with error code 408 * Added comment for 408 error * Added comments * Updated unit test cases for 400 error * Updated unit test case for create sdk client * Merged elif condition * Updated unit test cases * Tdl 15861 implement request timeout (#93) * TDL-15861 implement request timeout * added omment * removed unused imports * added comment * resolved confilcts with the base * resolved conflicts * resolved conflicts * resolved comments * updated unittests * added timeout for get calls * corrected a typo in comment * resolved comments * resolved comments Co-authored-by: prijendev <prijen.khokhani@crestdatasys.com> Co-authored-by: prijendev <prijen.khokhani@crestdatasys.com> * TDL-9902 removed the automatic fields from ad ext report (#85) * TDL-9902 removed the automatic fields from ad ext report * Added coverage report in artifact * revert back config.yml file * resolved comments Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> Co-authored-by: prijendev <88327452+prijendev@users.noreply.github.com> Co-authored-by: dbshah1212 <35164219+dbshah1212@users.noreply.github.com> Co-authored-by: prijendev <prijen.khokhani@crestdatasys.com>
Description of change
Removed the constraint imposed on AdExtensionDetailReport of having 'Ctrl', 'Clicks', and 'Impressions' from the required fields in the reports.py
Manual QA steps
Risks
Rollback steps