-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Google ads added fields and reports #9310
Google ads added fields and reports #9310
Conversation
working locally missing fields in click view and keyqord_view add date to campaigns add fields to click_view test
Thanks for the contribution @schlattk I request for the team to review it during the week. |
@@ -0,0 +1,16 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configured streams provided in json files should not be placed in the root of the connector.
I see they are not used in the code. So, you can put them under secrets/
directory which is in gitignore
.
source-google-ads/test_catalog_ad_group_ad_report.json
source-google-ads/ test_catalog_campaigns.json
source-google-ads/test_catalog_click_view.json
source-google-ads/test_catalog_click_view.json
source-google-ads/test_catalog_display_keyword_performance_report.json
source-google-ads/test_catalog_keyword_view.json
@@ -23,6 +23,7 @@ | |||
"shopping_performance_report": "shopping_performance_view", | |||
"user_location_report": "user_location_view", | |||
"click_view": "click_view", | |||
"keyword_view": "keyword_view" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keyword_view
stream was already added.
You should update your forked repo from the original repo. And resolve the Merge conflicts.
@@ -129,7 +129,7 @@ class Accounts(GoogleAdsStream): | |||
primary_key = "customer.id" | |||
|
|||
|
|||
class Campaigns(GoogleAdsStream): | |||
class Campaigns(IncrementalGoogleAdsStream): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configured stream "campaigns" should be updated in configured_catalog.json
, need to add incremental
mode and set sync_mode
to it, as below:
{
"stream": {
"name": "campaigns",
"json_schema": {},
"supported_sync_modes": ["full_refresh", "incremental"],
"source_defined_primary_key": [["campaign.id"]],
"source_defined_cursor": true,
"default_cursor_field": ["segments.date"]
},
"sync_mode": "incremental",
"destination_sync_mode": "overwrite",
"cursor_field": ["segments.date"]
}
@schlattk did you have time to look the comments made by @augan-rymkhan ? |
yes sorry for the delay @marcosmarxm @augan-rymkhan |
@augan-rymkhan could you have another look? |
@@ -248,6 +247,9 @@ | |||
}, | |||
"segments.year": { | |||
"type": ["null", "integer"] | |||
}, | |||
"segments.date": { | |||
"type": ["null", "integer"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schlattk Why segments.date
type is changed from "string"
to "integer"
and removed "format": "date"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@augan-rymkhan ok thanks if you could have another look
@@ -24,8 +24,12 @@ | |||
ClickView, | |||
DisplayKeywordPerformanceReport, | |||
DisplayTopicsPerformanceReport, | |||
<<<<<<< HEAD | |||
KeywordView, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schlattk stream KeywordView
is not defined anywhere. You should remove all the references to KeywordView
(resolving merge conflicts also), because this stream is implemented as KeywordReport
.
"type": ["null", "number"] | ||
}, | ||
"segments.date": { | ||
"type": ["null", "string"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to add "format": "date"
here
Campaigns(**incremental_stream_config), | ||
ClickView(**incremental_stream_config), | ||
<<<<<<< HEAD | ||
KeywordView(**incremental_stream_config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need in KeywordView
.
airbyte-integrations/connectors/source-google-ads/source_google_ads/source.py
Show resolved
Hide resolved
/test connector=connectors/source-google-ads
|
@augan-rymkhan I removed this new conflict |
@schlattk
It seems some added fields are not supported metrics.
<_InactiveRpcError of RPC that terminated with:\n\tstatus = StatusCode.INVALID_ARGUMENT\n\tdetails = "Request contains an invalid argument."\n\tdebug_error_string = "{"created":"@1645780794.933768829","description":"Error received from peer ipv4:216.58.207.202:443","file":"src/core/lib/surface/call.cc","file_line":903,"grpc_message":"Request contains an invalid argument.","grpc_status":3}"\n>, errors {\n error_code {\n query_error: PROHIBITED_SEGMENT_WITH_METRIC_IN_SELECT_OR_WHERE_CLAUSE\n }\n message: "Cannot select the following segments because at least one unsupported metric is found in SELECT or WHERE clause: \'segments.ad_network_type\'(unsupported metrics: \'historical_quality_score\')."\n}\nrequest_id: "REm0DyAHjwK4YuSl6iScIA"\n, 'REm0DyAHjwK4YuSl6iScIA')"}} |
@augan-rymkhan these tests run forever on my connection but could be an incorrect type on |
@schlattk Ok, but this does not fix that errors: |
@augan-rymkhan it appears to be this ? |
"ad_group_criterion.criterion_id": { | ||
"type": ["null", "integer"] | ||
}, | ||
"segments.ad_network_type": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schlattk Error happens because this field was added.
message: "Cannot select the following segments because at least one unsupported metric is found in SELECT or WHERE clause: \'segments.ad_network_type\'(unsupported metrics: \'historical_quality_score\')."\n}\nrequest_id: "hBx3G90_bSZ-rVpnBMQlxQ"\n, 'hBx3G90_bSZ-rVpnBMQlxQ')"}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works when read
command is run without this field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@augan-rymkhan ok but it is a supported field according to documentation and also it has been running live for a long time without any errors. What do you suggest is the solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schlattk It seems we can not query metrics.historical_quality_score
and segments.ad_network_type
together in keyword_report
stream.
In docs:
A segment in the SELECT clause is incompatible with a metric in the SELECT or WHERE clause.
For more you can read PROHIBITED_SEGMENT_WITH_METRIC_IN_SELECT_OR_WHERE_CLAUSE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes @augan-rymkhan that appears to be the problem. Ok, I removed the field I think we can most probably do without it in this report.
@@ -5,6 +5,9 @@ | |||
"ad_group_ad.ad.legacy_responsive_display_ad.accent_color": { | |||
"type": ["null", "string"] | |||
}, | |||
"ad_group.id": { | |||
"type": ["null", "string"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schlattk type must be "integer", not "string".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@augan-rymkhan let me know if any further changes are needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @schlattk
* combine all working locally missing fields in click view and keyqord_view add date to campaigns add fields to click_view test * configured_catalog campaigns * rem keywordview * conflicts * string type * remove network type * change ad group id to integer Co-authored-by: Konrad <konrad@PB-Mac8.local>
What
Add fields to several streams and update existing streams
How
Describe the solution
Recommended reading order
x.java
y.python
🚨 User Impact 🚨
Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/SUMMARY.md
docs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing./publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing./publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changesThis change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)