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

Trace Context for GoogleCloudLoggingFormatter #1877

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
"ext-mongodb": "Allow sending log messages to a MongoDB server (via driver)",
"mongodb/mongodb": "Allow sending log messages to a MongoDB server (via library)",
"aws/aws-sdk-php": "Allow sending log messages to AWS services like DynamoDB",
"google/cloud": "Allow inclusion of additional contextual information on GCP",
"rollbar/rollbar": "Allow sending log messages to Rollbar",
"ext-mbstring": "Allow to work properly with unicode symbols",
"ext-sockets": "Allow sending log messages to a Syslog server (via UDP driver)",
Expand Down
44 changes: 44 additions & 0 deletions src/Monolog/Formatter/GoogleCloudLoggingFormatter.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@
*/
final class GoogleCloudLoggingFormatter extends JsonFormatter
{
const CONTEXT_HEADER_FORMAT = '/([0-9a-fA-F]{32})(?:\/(\d+))?(?:;o=(\d+))?/';

private static ?string $traceID = null;

protected function normalizeRecord(LogRecord $record): array
{
$normalized = parent::normalizeRecord($record);
Expand All @@ -32,9 +36,49 @@
$normalized['severity'] = $normalized['level_name'];
$normalized['time'] = $record->datetime->format(DateTimeInterface::RFC3339_EXTENDED);

// Tag with Trace ID for request attribution
$normalized['logging.googleapis.com/trace'] = $this->getTraceID();
Copy link
Owner

Choose a reason for hiding this comment

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

Should this really be set at all if getTraceID returns null?


// Remove keys that are not used by GCP
unset($normalized['level'], $normalized['level_name'], $normalized['datetime']);

return $normalized;
}

private function getTraceID(): ?string
{
if (empty($this->traceID) && !empty($_SERVER['HTTP_X_CLOUD_TRACE_CONTEXT'])) {

Check failure on line 50 in src/Monolog/Formatter/GoogleCloudLoggingFormatter.php

View workflow job for this annotation

GitHub Actions / PHPStan (8.1)

Construct empty() is not allowed. Use more strict comparison.

Check failure on line 50 in src/Monolog/Formatter/GoogleCloudLoggingFormatter.php

View workflow job for this annotation

GitHub Actions / PHPStan (8.1)

Construct empty() is not allowed. Use more strict comparison.
$matched = preg_match(
self::CONTEXT_HEADER_FORMAT,
$_SERVER['HTTP_X_CLOUD_TRACE_CONTEXT'] ?? '',
$matches,
);

if (!$matched) {

Check failure on line 57 in src/Monolog/Formatter/GoogleCloudLoggingFormatter.php

View workflow job for this annotation

GitHub Actions / PHPStan (8.1)

Only booleans are allowed in a negated boolean, int|false given.
return null;
}

$projectID = $this->getProjectID();
if (empty($projectID)) {

Check failure on line 62 in src/Monolog/Formatter/GoogleCloudLoggingFormatter.php

View workflow job for this annotation

GitHub Actions / PHPStan (8.1)

Construct empty() is not allowed. Use more strict comparison.
return null;
}

$this->traceID = 'projects/'.$projectID.'/traces/'.strtolower($matches[1]);
}

return $this->traceID;
}

private function getProjectID(): ?string
{
if (isset($_SERVER['GOOGLE_CLOUD_PROJECT'])) {
return $_SERVER['GOOGLE_CLOUD_PROJECT'];
}

if (class_exists('\Google\Cloud\Core\Compute\Metadata')) {
Seldaek marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if (class_exists('\Google\Cloud\Core\Compute\Metadata')) {
if (class_exists('Google\Cloud\Core\Compute\Metadata')) {

return (new \Google\Cloud\Core\Compute\Metadata())->getProjectId();

Choose a reason for hiding this comment

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

also, it looks like the implementation of Google\Cloud\Core\Compute\Metadata::getProjectId() does not do caching, so maybe it's better to retrieve the project id just once at the constructor of this GoogleCloudLoggingFormatter as not to incur HTTP round trip every time there is a new log entry to format.

Copy link
Author

Choose a reason for hiding this comment

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

You'll hopefully see further up, we set the static property $this->traceID and once set, we always return that static value.

The project ID is only retrieved to construct that value the single time it is constructed, so hopefully this is already accounted for.

Choose a reason for hiding this comment

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

Caching $this->traceID may be fine if it's guaranteed that GoogleCloudLoggingFormatter is always created new for each request. I think this is the case with PHP app with nginx + php-fpm but may not be the case with other kind of deployment (like with Laravel Octane where objects are created just once when the app starts and reused across multiple requests).

I am not familiar enough with monolog + octane to confidently say whether Octane reuses the formatter object across different requests, but to be safe I would not cache the trace id but only the project id.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah it's probably safer not to cache, as retrieving the trace id doesn't seem that expensive.

Copy link
Owner

Choose a reason for hiding this comment

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

And I would definitely not cache this as a static property. Cache on an instance property then a new instance would at least refetch from a clean state. Static property will definitely cause issues in some envs.

}

return null;
}
}