-
Notifications
You must be signed in to change notification settings - Fork 14
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
Making GA4 imports work with a proxy #522
base: 5.x-dev
Are you sure you want to change the base?
Conversation
@AltamashShaikh Do these changes look like they should work to you? |
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.
@snake14 This should work, we can ask the user to apply this and confirm if it works as expected or not.
@snake14 No change, unfortunately. I'm pasting output from the console below. Please let me know if there are any specific debugging steps I could try that would help. I'll poke at it some today and see if I can figure anything out. I did confirm that setting the
|
For the record, with this PR the GA3 import still works, no regression. With a little debug I can see that the new code in AuthorizationGA4.php
|
Thank you @machinehum . Looking at the GuzzleHttp\Client you output, I'd say that looks correct. What does the plugin-GoogleAnalyticsImporter/vendor/prefixed/google/gax/src/CredentialsWrapper.php Line 110 in cad809f
base_uri value for GA4?
|
@snake14 Hm. In I did confirm that the conditional on line 51-53 is being entered. So it seems like the new form of setting the arguments isn't actually passing them through? I'm going to try some more debugging. I suppose you could do similar even without a proxy to see whether this version of |
With the change in this PR to AuthorizationGA4, the
results in CredentialsWrapper showing what looks like a good GuzzleHttp\Client object. However, the initial OAuth request still isn't using the proxy. The BetaAnalyticsDataGapicClient and AnalyticsAdminServiceGapicClient builders also allows for passing in a set of config arguments via |
Nice find @machinehum . I meant to pass the handler into the |
@snake14 Yeah, I did try the transport config eventually as well. That didn't make any difference either. Ended up spending 9 hours debugging this today. The documentation on the google PHP API side seems a little scattered. I did see that some of the modules of google-cloud-php, like BigTables, implement their own proxy settings. This makes me suspicious about whether the "official" solution is fully tested and working. Most of the threads I can find on it say "just use environment variables", heh. I opened an issue at googleapis/google-cloud-php#7274 to hopefully get some guidance. |
Thank you for all of your effort @machinehum ; it's greatly appreciated. I agree that it's hard to find a clear answer in Google's documentation. Pretty much all of my searches for using a proxy with Google's PHP API classes ended up pointing to answers for UA and not GA4. |
Hi @machinehum . I tried to follow the input that was provided on the issue you created. I tried to test the changes, but got some Guzzle error. If you get a chance to try out the latest changes and let me know how it goes, it would be greatly appreciated. |
Hi @machinehum . Reading your most recent comment, it looks like you received the same error that I did when I tried testing the suggested workaround. I'll keep watching that thread 👍 |
@machinehum . I made the most recently suggested change and it appears to fix the Guzzle error. Does this PR work correctly for your proxy now? |
@snake14 Thanks for keeping an eye on it! I'm continuing to test. Unfortunately it still doesn't use the proxy with your newest commits. But, I have some new insight. As recommended in that issue I created a test script that authenticates and makes a GET request to an API endpoint directly with the Guzzle client, without using the PHP libraries. I modified it slightly to use an OAuth client (like the importer does) instead of a service account. That test worked appropriately with and without the proxy. I tried inserting the same code into So, this points to an issue in the importer context. I did more exhaustive tests on the importer code, and found something interesting. I tried hard-coding client setup (just to rule out any issues in the chain of configs loading), and an AnalyticsAdmin API call in
Then I tried an import run from the console:
BUT, if the proxy in the code is set to a non-existent URL, this changes:
I replicated the same results by removing this hard-coded stuff and running the same tests with this branch, varying the proxy settings in All of this taken together is leading me to believe that before code execution gets to This call, wherever it is, times out if the https_proxy env var isn't set. If the env var is set to a non-existent URL, there's an immediate Guzzle/curl failure on connecting to that URL. If that env var is set to a valid proxy URL, then:
I've traced the code some but I'm not sure where that unknown OAuth attempt is coming from. You might have more luck. The other thing I've learned from all of this is that you should be able to test the issue without actually having a proxy server to test with -- if you create a Guzzle client with a non-existent proxy URL it should surface as a Guzzle/curl error. Create clients with different 'bad' proxy URLs in different places, and the error message will reveal which one failed because it includes the URL that it attempted to proxy through. Combine this with setting a non-existent URL in the Does this make sense? I'm not sure if I explained it well or if it's too wordy. |
Thank you @machinehum . I haven't been able to spend much time on this today, but I'll try to do some more debugging first thing next week. |
@machinehum It took some serious debugging, but I think I found where it's using the ENV variable instead of the defined proxy. In Google's CredentialsWrapper line 171, it doesn't pass the http handler like it does on line 175, so a default handler gets created. I tried passing Edit: Looks like the issue was fixed toward the end of April. I'll have to see if we can update to that version of the dependency or if it would be a breaking change. |
@snake14 Great catch! Thanks, and I'll be looking forward to any updates. |
@machinehum I'll keep working on the UI build failure, but I'm pretty sure that it's an issue in the test and not the dependency upgrade itself. Can you please test if things work correctly with your proxy? @AltamashShaikh Can you please review again since I upgraded multiple dependencies? |
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.
The import works fine for me 👍
@machinehum I think that this PR might finally be ready to go if it works for your proxy. Please let me know how your testing goes when you get a chance. |
Hi @machinehum . It's been a while, so I thought that I'd check in. I hope that you're doing well. Any chance you'll have an opportunity to test this PR soon? |
Description:
We previously added the ability for UA imports to work with a proxy. This is to make GA4 imports work with a proxy as well.
Fixes Issue: #521
Review