Providing http client to ReportDownloader causes duplicates in logs on consequent downloads

[mrpavlikov]

I have one MCC account and multiple managed ad accounts. I use single session to get reports for all ad accounts. So in loop I have something like this:

$reportDownloader = new ReportDownloader($session, null, $this->client);

Where $this->client is my own GuzzleHttp\Client. If I do so each call to report downloader produces $i + 1 log lines because before handlers keep stacking, so I get:

14:00:15 INFO      [app] clientCustomerId=CUSTOMER1 ITPub-Import/0.1 (AwApi-PHP, googleads-php-lib/44.0.0, PHP/7.3.11, GuzzleHttp/6.5.1, curl/7.64.0, ReportQueryBuilder) "POST /api/adwords/reportdownload/v201809 HTTP/1.1" Status: 200 


14:00:16 INFO      [app] clientCustomerId=CUSTOMER2 ITPub-Import/0.1 (AwApi-PHP, googleads-php-lib/44.0.0, PHP/7.3.11, GuzzleHttp/6.5.1, curl/7.64.0, ReportDownloader/string, ReportQueryBuilder) "POST /api/adwords/reportdownload/v201809 HTTP/1.1" Status: 200 
14:00:16 INFO      [app] clientCustomerId=CUSTOMER1 ITPub-Import/0.1 (AwApi-PHP, googleads-php-lib/44.0.0, PHP/7.3.11, GuzzleHttp/6.5.1, curl/7.64.0, ReportDownloader/string, ReportQueryBuilder) "POST /api/adwords/reportdownload/v201809 HTTP/1.1" Status: 200 

14:00:18 INFO      [app] clientCustomerId=CUSTOMER3 ITPub-Import/0.1 (AwApi-PHP, googleads-php-lib/44.0.0, PHP/7.3.11, GuzzleHttp/6.5.1, curl/7.64.0, ReportDownloader/string, ReportQueryBuilder) "POST /api/adwords/reportdownload/v201809 HTTP/1.1" Status: 200 
14:00:18 INFO      [app] clientCustomerId=CUSTOMER2 ITPub-Import/0.1 (AwApi-PHP, googleads-php-lib/44.0.0, PHP/7.3.11, GuzzleHttp/6.5.1, curl/7.64.0, ReportDownloader/string, ReportQueryBuilder) "POST /api/adwords/reportdownload/v201809 HTTP/1.1" Status: 200 
14:00:18 INFO      [app] clientCustomerId=CUSTOMER1 ITPub-Import/0.1 (AwApi-PHP, googleads-php-lib/44.0.0, PHP/7.3.11, GuzzleHttp/6.5.1, curl/7.64.0, ReportDownloader/string, ReportQueryBuilder) "POST /api/adwords/reportdownload/v201809 HTTP/1.1" Status: 200 

[fiboknacky]

Hello,

Thanks for reporting this, but ReportDownloader is meant to be used with only one customer ID, as exemplified in this example.

If your use case needs to re-use the HTTP Client object, probably you need to clone it outside ReportDownloader.

Best,
Knack

[mrpavlikov]

I don't think I get it. I make new instance of reporter for each customer ID, why should it be illegal?

And about HTTP Client: that means library should always accept clones since it changes the original handlers. If I try to use my http client after I've downloaded report, I'll have this new logger middleware running in my client, and I haven't asked for it.

[fiboknacky]

I don't think I get it. I make new instance of reporter for each customer ID, why should it be illegal?

It's not illegal. I didn't say that. :) What I say is the ReportDownloader is meant to be used with one customer ID at a time, so does the HTTP client.

And about HTTP Client: that means library should always accept clones since it changes the original handlers. If I try to use my http client after I've downloaded report, I'll have this new logger middleware running in my client, and I haven't asked for it.

Can't you clone the HTTP client before passing it to the ReportDownloader?
Or clear out all the middlewares before you use it for the next time? Where else do you use your HTTP client for?
I just want to understand your use case a bit more.

[mrpavlikov]

I could clone HTTP client before passing it, sure, but first of all it is not clear from documentation that my client will be modified. At second, if you look at my PR, provided client is only used to get its config and make new instance of library's client. The problem is that despite of config is being array, it points to an stack object, so when Google Library creates an new object it modifies original client's stack. So one should always clone client before passing it to library, why do so if we can (as in PR) just clone stack and return new client with new stack?

About my case: I have Symfony project where I build my http client once and use it for different api's. I also have some custom handlers there for profiling sake, desired settings like timeouts e.t.c. So I use one instance of Guzzle Client through application, just getting it from DI container and passing to different sdk's.

[mrpavlikov]

I'm kinda doing something like this Parallel Report Download, where multiple ReportDownloader's created, just passing custom http client.

[mrpavlikov]

And I don't see why one should use 1 client for 1 request, I feel like 1 client should be used for all requests, because why not, why clone what does not need to be cloned.

[fiboknacky]

Alright, I was trying to find if there are other solutions that wouldn't need cloning, but it seems not quite feasible without changing the way that the ReportDownloader and other utils work.
Thanks for your discussion. I've merged the PR, so this issue will be closed.

[mrpavlikov]

Thank you!