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

feat: add StdOutLogger and LoggingTrait #578

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
2416bec
Add the StdOutLogger and Logging Trait
Hectorhammett Sep 4, 2024
dbce7ca
Fix types and style
Hectorhammett Sep 4, 2024
6370690
fix typing for PSR-3 logger interface
Hectorhammett Sep 5, 2024
7ce3e58
Add logging to the Guzzle6HttpHandler
Hectorhammett Sep 9, 2024
ce1e7db
Change logic for getDefaultLogger
Hectorhammett Sep 9, 2024
ad4699b
Fix style issues
Hectorhammett Sep 9, 2024
b952d48
Fix phpstan typing for getJwtToken
Hectorhammett Sep 9, 2024
c80bdf6
Add a flag for disabling logger
Hectorhammett Sep 13, 2024
953efb7
Add logging to the invoke method for apiary support
Hectorhammett Sep 16, 2024
eba9861
Remove extra space
Hectorhammett Sep 16, 2024
975c564
Fix style issue on build method
Hectorhammett Sep 16, 2024
7e76353
Change logic for disabling logging
Hectorhammett Sep 20, 2024
ac4d686
Fix logic for disabling logging
Hectorhammett Sep 20, 2024
b770674
Add a function to log a gRPC status only
Hectorhammett Sep 25, 2024
566c414
Removed unused method
Hectorhammett Sep 25, 2024
8bb61a1
Fix style for LoggingTrait
Hectorhammett Sep 25, 2024
fa851c2
Fix typing
Hectorhammett Sep 25, 2024
600a947
Updated type annotations
Hectorhammett Sep 25, 2024
3cc71a4
Change logic for logging responses to exclude the info log
Hectorhammett Sep 27, 2024
8699369
Change logic for logResponse
Hectorhammett Sep 27, 2024
fb0ac53
Handle false for the case when json_encode returns false
Hectorhammett Oct 8, 2024
56d3881
Fix serialization bug
Hectorhammett Oct 16, 2024
202890d
Add support for logger in
Hectorhammett Oct 18, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
"guzzlehttp/guzzle": "^7.4.5",
"guzzlehttp/psr7": "^2.4.5",
"psr/http-message": "^1.1||^2.0",
"psr/cache": "^2.0||^3.0"
"psr/cache": "^2.0||^3.0",
"psr/log": "^3.0"
},
"require-dev": {
"guzzlehttp/promises": "^2.0",
Expand Down
47 changes: 44 additions & 3 deletions src/ApplicationDefaultCredentials.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,15 @@
use Google\Auth\Credentials\ServiceAccountCredentials;
use Google\Auth\HttpHandler\HttpClientCache;
use Google\Auth\HttpHandler\HttpHandlerFactory;
use Google\Auth\Logging\StdOutLogger;
use Google\Auth\Middleware\AuthTokenMiddleware;
use Google\Auth\Middleware\ProxyAuthTokenMiddleware;
use Google\Auth\Subscriber\AuthTokenSubscriber;
use GuzzleHttp\Client;
use InvalidArgumentException;
use PHPUnit\TextUI\XmlConfiguration\Logging\Logging;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't used - we should remove it

That's (partly) the reason the CS tests are failing

https://github.com/googleapis/google-auth-library-php/actions/runs/11411641502/job/31756272056

use Psr\Cache\CacheItemPoolInterface;
use Psr\Log\LoggerInterface;

/**
* ApplicationDefaultCredentials obtains the default credentials for
Expand Down Expand Up @@ -69,6 +72,8 @@
*/
class ApplicationDefaultCredentials
{
private const SDK_DEBUG_FLAG = 'GOOGLE_SDK_DEBUG_LOGGING';

/**
* @deprecated
*
Expand Down Expand Up @@ -145,7 +150,8 @@ public static function getMiddleware(
* user-defined scopes exist, expressed either as an Array or as a
* space-delimited string.
* @param string $universeDomain Specifies a universe domain to use for the
* calling client library
* calling client library.
* @param null|false|LoggerInterface $logger A PSR3 compliant LoggerInterface.
*
* @return FetchAuthTokenInterface
* @throws DomainException if no implementation can be obtained.
Expand All @@ -157,7 +163,8 @@ public static function getCredentials(
CacheItemPoolInterface $cache = null,
$quotaProject = null,
$defaultScope = null,
string $universeDomain = null
string $universeDomain = null,
null|false|LoggerInterface $logger = null,
) {
$creds = null;
$jsonKey = CredentialsLoader::fromEnv()
Expand All @@ -170,7 +177,7 @@ public static function getCredentials(
HttpClientCache::setHttpClient($client);
}

$httpHandler = HttpHandlerFactory::build($client);
$httpHandler = HttpHandlerFactory::build($client, $logger);
}

if (is_null($quotaProject)) {
Expand Down Expand Up @@ -321,6 +328,40 @@ public static function getIdTokenCredentials(
return $creds;
}

/**
* Returns a StdOutLogger instance
*
* @return null|LoggerInterface
*/
public static function getDefaultLogger(): null|LoggerInterface
{
$loggingFlag = getenv(self::SDK_DEBUG_FLAG);

// Env var is not set
if (!is_string($loggingFlag)) {
if (is_array($loggingFlag)) {
trigger_error('The ' . self::SDK_DEBUG_FLAG . ' is set, but it is set to another value than false, true, 0 or 1. Logging is disabled');
return null;
}

return null;
}
Comment on lines +341 to +348
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this block - there is no way for the logging flag to be an array, as that only happens if you call getenv with no arguments (see here). So the only thing this is doing is returning null when the env var isn't set, which is what we also do for 0 and false.

Why not combine this with the validation below? (see suggestion below)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember having a specific issue that I needed to handle but I can't quite remember what was it exactly. Although it might have been related to PHPSTAN mentioning that it can return an array? But I am unsure.

Still let me take a look in order to make it cleaner :)


$loggingFlag = strtolower($loggingFlag);

// Env Var is not true
if ($loggingFlag !== 'true' && $loggingFlag !== '1') {
// Env var is set to a non valid value
if ($loggingFlag !== 'false' && $loggingFlag !== '0') {
trigger_error('The ' . self::SDK_DEBUG_FLAG . ' is set, but it is set to another value than false, true, 0 or 1. Logging is disabled');
}

return null;
}

return new StdOutLogger();
Comment on lines +352 to +362
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having lots of !== is a bit hard to read. What about something like this?

NOTE: You may want to handle the case of empty string (e.g. someone sets GOOGLE_SDK_DEBUG_LOGGING= in their environment) to have the same behavior, which is why I included the empty string in the array.

Suggested change
// Env Var is not true
if ($loggingFlag !== 'true' && $loggingFlag !== '1') {
// Env var is set to a non valid value
if ($loggingFlag !== 'false' && $loggingFlag !== '0') {
trigger_error('The ' . self::SDK_DEBUG_FLAG . ' is set, but it is set to another value than false, true, 0 or 1. Logging is disabled');
}
return null;
}
return new StdOutLogger();
// Env Var is true
if ($loggingFlag === 'true' || $loggingFlag === '1') {
return StdOutLogger();
}
// Env Var is false or not specified
if (in_array($loggingFlag, [null, '', 'false', '0'], true)) {
return null;
}
// Env var is set to a non valid value
trigger_error('The ' . self::SDK_DEBUG_FLAG . ' is set, but it is set to another value than false, true, 0 or 1. Logging is disabled');
// still return null because we don't want debugging to halt execution
return null;

I personally think this reads a lot cleaner

}

/**
* @return string
*/
Expand Down
84 changes: 81 additions & 3 deletions src/HttpHandler/Guzzle6HttpHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,35 @@
*/
namespace Google\Auth\HttpHandler;

use Google\Auth\Logging\LogEvent;
use Google\Auth\Logging\LoggingTrait;
use GuzzleHttp\ClientInterface;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
use Psr\Log\LoggerInterface;

class Guzzle6HttpHandler
{
use LoggingTrait;

/**
* @var ClientInterface
*/
private $client;

/**
* @var null|LoggerInterface
*/
private $logger;

/**
* @param ClientInterface $client
* @param null|LoggerInterface $logger
*/
public function __construct(ClientInterface $client)
public function __construct(ClientInterface $client, LoggerInterface $logger = null)
{
$this->client = $client;
$this->logger = $logger;
}

/**
Expand All @@ -44,7 +56,38 @@ public function __construct(ClientInterface $client)
*/
public function __invoke(RequestInterface $request, array $options = [])
{
return $this->client->send($request, $options);
$requestEvent = null;

if ($this->logger) {
$requestEvent = new LogEvent();

$requestEvent->method = $request->getMethod();
$requestEvent->url = $request->getUri()->__toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolute nit

Suggested change
$requestEvent->url = $request->getUri()->__toString();
$requestEvent->url = (string) $request->getUri();

$requestEvent->headers = $request->getHeaders();
$requestEvent->payload = $request->getBody()->getContents();
$requestEvent->retryAttempt = $options['retryAttempt'] ?? null;
$requestEvent->serviceName = $options['serviceName'] ?? null;
$requestEvent->clientId = spl_object_id($this->client);
$requestEvent->requestId = spl_object_id($request);

$this->logRequest($requestEvent);
}

$response = $this->client->send($request, $options);

if ($this->logger) {
$responseEvent = new LogEvent($requestEvent->timestamp);

$responseEvent->headers = $response->getHeaders();
$responseEvent->payload = $response->getBody()->getContents();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could potentially be huge, depending on the response - is that still what we want to do? Maybe we should have some kind of checks to only send the first # of bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there was a discussion about truncating the data for this and there is no definite conclusion, but one idea is to truncate the length and be configurable via the StdOutLogger constructor.

$responseEvent->status = $response->getStatusCode();
$responseEvent->clientId = $requestEvent->clientId;
$responseEvent->requestId = $requestEvent->requestId;

$this->logResponse($responseEvent);
}

return $response;
}

/**
Expand All @@ -57,6 +100,41 @@ public function __invoke(RequestInterface $request, array $options = [])
*/
public function async(RequestInterface $request, array $options = [])
{
return $this->client->sendAsync($request, $options);
$requestEvent = null;

if ($this->logger) {
$requestEvent = new LogEvent();

$requestEvent->method = $request->getMethod();
$requestEvent->url = $request->getUri()->__toString();
$requestEvent->headers = $request->getHeaders();
$requestEvent->payload = $request->getBody()->getContents();
$requestEvent->retryAttempt = $options['retryAttempt'] ?? null;
$requestEvent->serviceName = $options['serviceName'] ?? null;
$requestEvent->clientId = spl_object_id($this->client);
$requestEvent->requestId = spl_object_id($request);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this code is duplicated above, why not have private methods like logHttpRequest($request) and logHttpResponse($response, $requestEvent)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my mind, formatting the LogEvent and filling that data is not duty of the log function. The log function takes the generic log object and turns it into a json string to the docs specification, that's why I decided to have this code "duplicated", which in my mind is not really "duplicated" but is basically the same, mostly because one is for the response and one is for the request, and I did it this way in order to keep them separate from the logging logic.

I also thought perhaps a function to make a logEvent but voted against it as it would have to be flexible enough to acomodate the small differences between response and request but it can definitely be done.


$this->logRequest($requestEvent);
}

$promise = $this->client->sendAsync($request, $options);

if ($this->logger) {
$promise->then(function (ResponseInterface $response) use ($requestEvent) {
$responseEvent = new LogEvent($requestEvent->timestamp);

$responseEvent->headers = $response->getHeaders();
$responseEvent->payload = $response->getBody()->getContents();
$responseEvent->status = $response->getStatusCode();
$responseEvent->clientId = $requestEvent->clientId;
$responseEvent->requestId = $requestEvent->requestId;

$this->logResponse($responseEvent);

return $response;
});
}

return $promise;
}
}
18 changes: 15 additions & 3 deletions src/HttpHandler/HttpHandlerFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,28 @@
*/
namespace Google\Auth\HttpHandler;

use Google\Auth\ApplicationDefaultCredentials;
use GuzzleHttp\BodySummarizer;
use GuzzleHttp\Client;
use GuzzleHttp\ClientInterface;
use GuzzleHttp\HandlerStack;
use GuzzleHttp\Middleware;
use Psr\Log\LoggerInterface;

class HttpHandlerFactory
{
/**
* Builds out a default http handler for the installed version of guzzle.
*
* @param ClientInterface $client
* @param null|false|LoggerInterface $logger
* @return Guzzle6HttpHandler|Guzzle7HttpHandler
* @throws \Exception
*/
public static function build(ClientInterface $client = null)
public static function build(
ClientInterface $client = null,
null|false|LoggerInterface $logger = null,
)
{
if (is_null($client)) {
$stack = null;
Expand All @@ -45,6 +51,12 @@ public static function build(ClientInterface $client = null)
$client = new Client(['handler' => $stack]);
}

if ($logger === false) {
$logger = null;
} else {
$logger = $logger ?? ApplicationDefaultCredentials::getDefaultLogger();
}
Comment on lines +54 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is 100% not important, if you don't like it please disregard. I only suggest it because it sets the variable in one statement, so it might be considered a little easier to follow (but that's entirely subjective)

Suggested change
if ($logger === false) {
$logger = null;
} else {
$logger = $logger ?? ApplicationDefaultCredentials::getDefaultLogger();
}
$logger = ($logger === false)
? null
: $logger ?? ApplicationDefaultCredentials::getDefaultLogger();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't love the ?? inside a ternary, but I can see the appeal.


$version = null;
if (defined('GuzzleHttp\ClientInterface::MAJOR_VERSION')) {
$version = ClientInterface::MAJOR_VERSION;
Expand All @@ -54,9 +66,9 @@ public static function build(ClientInterface $client = null)

switch ($version) {
case 6:
return new Guzzle6HttpHandler($client);
return new Guzzle6HttpHandler($client, $logger);
case 7:
return new Guzzle7HttpHandler($client);
return new Guzzle7HttpHandler($client, $logger);
default:
throw new \Exception('Version not supported');
}
Expand Down
Loading
Loading