-
Notifications
You must be signed in to change notification settings - Fork 191
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
base: main
Are you sure you want to change the base?
Conversation
4122fb0
to
6370690
Compare
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; |
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.
This isn't used - we should remove it
That's (partly) the reason the CS tests are failing
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; | ||
} |
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.
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)
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.
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 :)
$requestEvent = new LogEvent(); | ||
|
||
$requestEvent->method = $request->getMethod(); | ||
$requestEvent->url = $request->getUri()->__toString(); |
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.
absolute nit
$requestEvent->url = $request->getUri()->__toString(); | |
$requestEvent->url = (string) $request->getUri(); |
$responseEvent = new LogEvent($requestEvent->timestamp); | ||
|
||
$responseEvent->headers = $response->getHeaders(); | ||
$responseEvent->payload = $response->getBody()->getContents(); |
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.
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?
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.
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.
$requestEvent->retryAttempt = $options['retryAttempt'] ?? null; | ||
$requestEvent->serviceName = $options['serviceName'] ?? null; | ||
$requestEvent->clientId = spl_object_id($this->client); | ||
$requestEvent->requestId = spl_object_id($request); |
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.
since this code is duplicated above, why not have private methods like logHttpRequest($request)
and logHttpResponse($response, $requestEvent)
?
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.
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.
|
||
/** | ||
* @param array<mixed> $headers | ||
* @return null|array<mixed, mixed> |
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.
we can make this a little smarter
* @return null|array<mixed, mixed> | |
* @return null|array<string, string|false> |
$stringifiedEvent = json_encode($debugEvent); | ||
|
||
// There was an error stringifying the event, return to not break execution | ||
if ($stringifiedEvent === false) { |
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.
do we still want to try to log the infoEvent even if the debug event fails? for instance do this instead?
if ($stringifiedEvent !== false) {
$this->logger->debug($stringifiedEvent);
}
* @return int | ||
* @throws InvalidArgumentException | ||
*/ | ||
private function getLevelMap(string $levelName): int |
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.
nit: to me getLevelMap
would return the levelMapping
variable. What if we instead called it getLevelNumber
or getLevelFromName
?
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.
I like it, let me change it.
$this->assertEquals($expectedString . "\n", $buffer); | ||
|
||
ob_end_clean(); | ||
} |
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.
Instead of usign ob_start
and ob_end_clean
, use expectOutputString
. See https://docs.phpunit.de/en/10.5/writing-tests-for-phpunit.html#testing-output for more info
// 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(); |
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.
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.
// 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
related: googleapis/gax-php#585
Add logging support to the HTTP calls for easy debugging.