Skip to content
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

Set user agent for requests #73

Closed
danepowell opened this issue Jun 10, 2020 · 4 comments
Closed

Set user agent for requests #73

danepowell opened this issue Jun 10, 2020 · 4 comments

Comments

@danepowell
Copy link
Collaborator

danepowell commented Jun 10, 2020

Is your feature request related to a problem? Please describe.
Currently, Cloud API requests from Acquia PHP SDK have a generic, default user agent string provided by Guzzle, i.e. GuzzleHttp/6.5.4 curl/7.68.0 PHP/7.4.3. This makes it difficult to monitor and debug failing requests from the server side, and trace them back to projects using Acquia PHP SDK.

Describe the solution you'd like
A unique identifier prepended to the user agent string, such as acquia-php-sdk-v2, and/or the ability for downstream projects to set their own user agent string.

Describe alternatives you've considered
I can't think of any. The current string isn't unique enough on its own to be useful, and there's no way I can see to override it from a downstream/consuming project.

If you want a more extensive solution, though, you could expose all of the headers to modification by consuming projects, not just the user-agent.

Additional context
I'm happy to help with a PR if you can let me know what approach you prefer. Thanks!

@typhonius
Copy link
Owner

That's a pretty cool idea - I'm a big fan of open data so will Acquia be able to publish some analytics/stats about user agents hitting the API?

It looks like the easiest solution here is to alter how we instantiate the Guzzle client. I'll have a think about the right user agent although probably some combination of the library name plus the version in use as this may assist in debugging customers with different versions to support staff.

I haven't dug into it to check, but dependent libraries might be able to use the addOption method to add their own user agent. This creates an array that gets used when creating the Guzzle object. I use it in Acquia Cli over here to pass specific parameters required by Guzzle for downloading files: https://github.com/typhonius/acquia_cli/blob/b043a3204cc9b8a1557764ede5f2bfe2328d394a/src/Commands/DbBackupCommand.php#L238-L244

@typhonius
Copy link
Owner

typhonius commented Jun 10, 2020

From http://docs.guzzlephp.org/en/stable/request-options.html#headers dependent libraries would probably do something like:

$client->addOption('headers', ['User-Agent' => "name/version"]);

Indeed, doing the following on AcquiaCli provides this logfile entry.

$client->addOption('headers', ['User-Agent' => sprintf("%s/%s (https://github.com/typhonius/acquia_cli)", self::NAME, $version)]);
api.adammalone.net:443 127.0.0.1 - - [10/Jun/2020:13:34:17 +0000] "GET /applications HTTP/1.1" 200 2568 "-" "AcquiaCli/2.0.7-dev (https://github.com/typhonius/acquia_cli)"

One final question I would have is how would be preferred to combine user agents. Doing a bit of reading online, I couldn't see a particular standard one way or the other, but some people do combine multiple agents with semi-colons so we could replicate that too.

@danepowell
Copy link
Collaborator Author

danepowell commented Jun 10, 2020

Ah, snap... I don't know how I missed $client->addOption('headers'). That works exactly as expected!

I'm satisfied with that as a solution, although perhaps you still want to set a default.

I also could not find any standard for user agent strings like this, and for other projects I've just set the client name and version as in your example. Prepending the client while leaving the guzzle info might also be useful.

I'm not sure what we could publish in terms of usage but let me know if you're looking for something in particular.

@typhonius
Copy link
Owner

I've just added a PR for this one too. Would be great if I could get your eyes on #74 in case it would cause any unforeseen issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants