search

Matatika / tap-googleads

0 stars 11 forks source link

Adding conversion and Ad Groups Historical Performance stream #34

Closed acarter24 closed 1 month ago

acarter24 commented 1 month ago

Couple of extra streams I found useful:

ReubenFrankel commented 1 month ago

Hi @acarter24, just a quick update to let you know that we are discussing our policy on accepting PRs for new streams like this. Since streams in this tap are essentially just wrappers for "arbitrary" queries to the Google Ads API, we want to be sure they are generally useful and reasonably unopinionated. Not saying that is the case here - we just need to improve our own Google Ads testing environment first in order to be able to test and make informed decisions on these kinds of changes.

acarter24 commented 1 month ago

Hi @acarter24, just a quick update to let you know that we are discussing our policy on accepting PRs for new streams like this. Since streams in this tap are essentially just wrappers for "arbitrary" queries to the Google Ads API, we want to be sure they are generally useful and reasonably unopinionated. Not saying that is the case here - we just need to improve our own Google Ads testing environment first in order to be able to test and make decisions on these kinds of changes.

Makes sense.

I think there are a couple of similar taps which support a json file as a config where you can pass different report configurations, rather than proliferating streams in python files. I'm thinking google analytics possibly?

ReubenFrankel commented 1 month ago

I think there are a couple of similar taps which support a json file as a config where you can pass different report configurations, rather than proliferating streams in python files. I'm thinking google analytics possibly?

Yeah, that might be good to add at some point as well. Problem then is it becomes a configuration nightmare for people who don't know what they want initially, so I think there's definitely still a case to keep some default streams - perhaps they just become the default config though...

ReubenFrankel commented 1 month ago

Just to confirm, I assume you are not blocked by this PR staying open for the time being? Given that you have your own fork obviously, and I've seen you pretty active in the Meltano Slack so you probably know how to reconfigure your pip_url! :sweat_smile:

acarter24 commented 1 month ago

https://github.com/MeltanoLabs/tap-google-analytics/blob/main/tap_google_analytics/defaults/default_report_definition.json

For reference, happy to leave the pr open for now. Yes I have my own fork(s)!

acarter24 commented 1 month ago

I'm seeing a few type validation errors (decimal vs integer) occuring with this code so maybe worth closing for now?

I also am working up some performance max queries which have asset groups rather than ad groups. Maybe better to have an open issue or discussion where I can share streams I have developed for folks who want to maintain their own fork?

ReubenFrankel commented 1 month ago

@acarter24 Thanks for the suggestion - I made a "Custom streams" discussion category, so feel free to drop those there. If there is enough interest in particular custom streams, we can eventually integrate them as first-party streams.

Still very on board with the custom streams as config feature as well, so that would be nice to get done at some point to remove the prerequisite of forking the tap repo.