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

Enhance plugin logging to provide more context information and request IDs #9853

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
1854c5e
Initial attempt to add request IDs and context to the messages
dmvrtx Dec 2, 2024
e02b9fd
Split responsibilities by adding a Logger Context management class
dmvrtx Dec 3, 2024
6a97436
Merge branch 'develop' into dev/hack-logging-enhancements
dmvrtx Dec 3, 2024
62a094d
Update type hints
dmvrtx Dec 3, 2024
c739949
Example usage and simplification of checks
dmvrtx Dec 3, 2024
f48b470
Log context only if it is updated, improve log line readability, add …
dmvrtx Dec 3, 2024
82602b9
Minor improvements and one more suggested application
dmvrtx Dec 3, 2024
819ef17
Fix unit tests after moving environment data into context
dmvrtx Dec 4, 2024
b70f125
Merge branch 'develop' into dev/hack-logging-enhancements
dmvrtx Dec 4, 2024
708e13f
Initial test suite for logger context
dmvrtx Dec 4, 2024
0f697a6
Merge branch 'develop' into dev/hack-logging-enhancements
dmvrtx Dec 5, 2024
7f5955d
Test both options of working with Logger Context
dmvrtx Dec 5, 2024
fdbcfd7
Update tests to ensure full code paths coverage
dmvrtx Dec 5, 2024
e4b1b11
Better handling JSON encoding errors
dmvrtx Dec 5, 2024
6dd09ed
Fix incorrect class usage
dmvrtx Dec 5, 2024
7e20321
Changelog entry
dmvrtx Dec 5, 2024
3927d38
Merge branch 'develop' into dev/hack-logging-enhancements
dmvrtx Dec 5, 2024
d16d5c5
Group logging of the request arguments
dmvrtx Dec 6, 2024
c47ea0c
Ensure data is redacted before being logged and update tests
dmvrtx Dec 6, 2024
9b5374f
Merge branch 'develop' into dev/hack-logging-enhancements
dmvrtx Dec 6, 2024
13933bb
Add indicator that object data is JSON-encoded and remove redundant m…
dmvrtx Dec 6, 2024
c2a0813
Merge branch 'develop' into dev/hack-logging-enhancements
dmvrtx Dec 12, 2024
269930d
Merge branch 'develop' into dev/hack-logging-enhancements
dmvrtx Dec 16, 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
4 changes: 4 additions & 0 deletions changelog/dev-hack-logging-enhancements
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: dev

Enhance log file format to provide more information about the request flow.
3 changes: 3 additions & 0 deletions includes/admin/class-wc-rest-payments-timeline-controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

defined( 'ABSPATH' ) || exit;

use WCPay\Logger_Context;

/**
* REST controller for the timeline, which includes all events related to an intention.
*/
Expand Down Expand Up @@ -40,6 +42,7 @@ public function register_routes() {
*/
public function get_timeline( $request ) {
$intention_id = $request->get_param( 'intention_id' );
Logger_Context::set_value( 'intention_id', $intention_id );
dmvrtx marked this conversation as resolved.
Show resolved Hide resolved
return $this->forward_request( 'get_timeline', [ $intention_id ] );
}
}
38 changes: 38 additions & 0 deletions includes/class-logger-context.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<?php
/**
* Class LoggerContext
*
* @package WooCommerce\Payments
*/

namespace WCPay;

use WCPay\Internal\LoggerContext as InternalLoggerContext;

defined( 'ABSPATH' ) || exit; // block direct access.

/**
* A wrapper class for accessing LoggerContext as a singletone.
*/
class Logger_Context {
/**
* Initialises the logger context.
*
* @return void
*/
public static function init() {
wcpay_get_container()->get( InternalLoggerContext::class )->init();
dmvrtx marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Sets a context value.
*
* @param string $key The key to set.
* @param string|int|float|bool|null $value The value to set. Null removes value.
*
* @return void
*/
public static function set_value( $key, $value ) {
wcpay_get_container()->get( InternalLoggerContext::class )->set_value( $key, $value );
Comment on lines +26 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing to do with this PR in particular, but when we started the re-engineering project, and added src with the DI container, I was really hoping that we'd be able to refactor the whole plugin to work that way. Thus this feels like a hack, but I understand it is necessary.

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 wasn't aware about it, but sounds interesting. I think we way src is made to work is much easier to follow than the long lists of include or require operators in the other parts of plugin code.

}
}
41 changes: 29 additions & 12 deletions includes/class-logger.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,21 @@ class Logger {
* we need this to access the plugins debug setting to figure out if the setting
* is turned on.
*
* @param string $message Log message.
* @param string $message Log message.
*
* @param string $level One of the following:
* 'emergency': System is unusable.
* 'alert': Action must be taken immediately.
* 'critical': Critical conditions.
* 'error': Error conditions.
* 'warning': Warning conditions.
* 'notice': Normal but significant condition.
* 'info': Informational messages.
* 'debug': Debug-level messages.
* @param string $level One of the following:
* 'emergency': System is unusable.
* 'alert': Action must be taken immediately.
* 'critical': Critical conditions.
* 'error': Error conditions.
* 'warning': Warning conditions.
* 'notice': Normal but significant condition.
* 'info': Informational messages.
* 'debug': Debug-level messages.
* @param array<string, string> $context Context data.
*/
public static function log( $message, $level = 'info' ) {
wcpay_get_container()->get( InternalLogger::class )->log( $message );
public static function log( $message, $level = 'info', $context = [] ) {
wcpay_get_container()->get( InternalLogger::class )->log( $message, $level, $context );
RadoslavGeorgiev marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -119,4 +120,20 @@ public static function info( $message ) {
public static function debug( $message ) {
self::log( $message, 'debug' );
}

/**
* Formats an object for logging.
*
* @param string $label Label for the object.
* @param mixed $object Object to format.
* @return string
*/
public static function format_object( $label, $object ) {
try {
$encoded = wp_json_encode( $object, JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE | JSON_THROW_ON_ERROR );
Copy link
Contributor

Choose a reason for hiding this comment

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

Small suggestion here, but could we add a string like JSON: at the beginning here? Then the parser would not be required to guess whether it is JSON that should be parsed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 13933bb

} catch ( \JsonException $e ) {
return sprintf( 'Error encoding object "%s": %s', $label, $e->getMessage() );
}
return sprintf( '%s: %s', $label, $encoded );
}
}
3 changes: 3 additions & 0 deletions includes/class-wc-payments.php
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ public static function init() {
include_once __DIR__ . '/class-wc-payments-account.php';
include_once __DIR__ . '/class-wc-payments-customer-service.php';
include_once __DIR__ . '/class-logger.php';
include_once __DIR__ . '/class-logger-context.php';
include_once __DIR__ . '/class-session-rate-limiter.php';
include_once __DIR__ . '/class-wc-payment-gateway-wcpay.php';
include_once __DIR__ . '/class-wc-payments-checkout.php';
Expand Down Expand Up @@ -509,6 +510,8 @@ public static function init() {
include_once __DIR__ . '/compat/multi-currency/class-wc-payments-currency-manager.php';
include_once __DIR__ . '/class-duplicates-detection-service.php';

wcpay_get_container()->get( \WCPay\Internal\LoggerContext::class )->init();

self::$woopay_checkout_service = new Checkout_Service();
self::$woopay_checkout_service->init();

Expand Down
37 changes: 12 additions & 25 deletions includes/wc-payment-api/class-wc-payments-api-client.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use WCPay\Fraud_Prevention\Fraud_Prevention_Service;
use WCPay\Fraud_Prevention\Buyer_Fingerprinting_Service;
use WCPay\Logger;
use WCPay\Logger_Context;
use Automattic\WooCommerce\Admin\API\Reports\Customers\DataStore;
use WCPay\Constants\Currency_Code;
use WCPay\Database_Cache;
Expand Down Expand Up @@ -2098,33 +2099,22 @@ protected function request( $params, $api, $method, $is_site_specific = true, $u
}
}

$env = [];
$env['WP_User'] = is_user_logged_in() ? wp_get_current_user()->user_login : 'Guest (non logged-in user)';
$env['HTTP_REFERER'] = sanitize_text_field( wp_unslash( $_SERVER['HTTP_REFERER'] ?? '--' ) );
$env['HTTP_USER_AGENT'] = sanitize_text_field( wp_unslash( $_SERVER['HTTP_USER_AGENT'] ?? '--' ) );
$env['REQUEST_URI'] = sanitize_text_field( wp_unslash( $_SERVER['REQUEST_URI'] ?? '--' ) );
$env['DOING_AJAX'] = defined( 'DOING_AJAX' ) && DOING_AJAX;
$env['DOING_CRON'] = defined( 'DOING_CRON' ) && DOING_CRON;
$env['WP_CLI'] = defined( 'WP_CLI' ) && WP_CLI;
Logger::log(
'ENVIRONMENT: '
. var_export( $env, true ) // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_var_export
);
Logger_Context::set_value( 'WP_User', is_user_logged_in() ? wp_get_current_user()->user_login : 'Guest (non logged-in user)' );
Logger_Context::set_value( 'HTTP_REFERER', sanitize_text_field( wp_unslash( $_SERVER['HTTP_REFERER'] ?? '--' ) ) );
Logger_Context::set_value( 'HTTP_USER_AGENT', sanitize_text_field( wp_unslash( $_SERVER['HTTP_USER_AGENT'] ?? '--' ) ) );
Logger_Context::set_value( 'REQUEST_URI', sanitize_text_field( wp_unslash( $_SERVER['REQUEST_URI'] ?? '--' ) ) );
Logger_Context::set_value( 'DOING_AJAX', defined( 'DOING_AJAX' ) && DOING_AJAX );
Logger_Context::set_value( 'DOING_CRON', defined( 'DOING_CRON' ) && DOING_CRON );
Logger_Context::set_value( 'WP_CLI', defined( 'WP_CLI' ) && WP_CLI );

$headers = apply_filters( 'wcpay_api_request_headers', $headers );
Logger::log( "REQUEST $method $redacted_url" );
Logger::log(
'HEADERS: '
. var_export( $headers, true ) // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_var_export
);
Logger::log( Logger::format_object( 'HEADERS', $headers ) );

if ( null !== $body ) {
Logger::log(
'BODY: '
. var_export( $redacted_params, true ) // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_var_export
);
Logger::log( Logger::format_object( 'BODY', $redacted_params ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have the context, do we need to log things here as separate entries?

I imagine that we could store everything in context, and then only call log() once, either for the error, or successful response.

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 think it can be done in the next optimisation. I see the point, that we can log one Request object, but I'd do that if we use a similar object internally. However I think we can follow the structure of remote_request arguments when logging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in d16d5c5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After implementing the change I've noticed that we log redacted version of the arguments. I will update it, to follow the same structure, but use redacted data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in c47ea0c

}

$headers = apply_filters( 'wcpay_api_request_headers', $headers );
$stop_trying_at = time() + self::API_TIMEOUT_SECONDS;
$retries = 0;
$retries_limit = array_key_exists( 'Idempotency-Key', $headers ) ? self::API_RETRIES_LIMIT : 0;
Expand Down Expand Up @@ -2196,10 +2186,7 @@ protected function request( $params, $api, $method, $is_site_specific = true, $u
$response_body = $response;
}

Logger::log(
'RESPONSE: '
. var_export( WC_Payments_Utils::redact_array( $response_body, self::API_KEYS_TO_REDACT ), true ) // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_var_export
);
Logger::log( Logger::format_object( 'RESPONSE', WC_Payments_Utils::redact_array( $response_body, self::API_KEYS_TO_REDACT ) ) );

return $response_body;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use WCPay\Core\Mode;
use WCPay\Internal\DependencyManagement\AbstractServiceProvider;
use WCPay\Internal\Logger;
use WCPay\Internal\LoggerContext;
use WCPay\Internal\Proxy\HooksProxy;
use WCPay\Internal\Proxy\LegacyProxy;
use WCPay\Internal\Service\Level3Service;
Expand All @@ -30,6 +31,7 @@ class GenericServiceProvider extends AbstractServiceProvider {
*/
protected $provides = [
Logger::class,
LoggerContext::class,
OrderService::class,
Level3Service::class,
TranslationsLoader::class,
Expand All @@ -47,6 +49,8 @@ public function register(): void {
->addArgument( 'wc_get_logger' )
->addArgument( Mode::class );

$container->addShared( LoggerContext::class );

$container->addShared( OrderService::class )
->addArgument( WC_Payments_Order_Service::class )
->addArgument( LegacyProxy::class )
Expand Down
26 changes: 14 additions & 12 deletions src/Internal/Logger.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,22 +48,24 @@ public function __construct( WC_Logger_Interface $wc_logger, Mode $mode ) {
/**
* Add a log entry.
*
* @param string $message Log message.
* @param string $level One of the following:
* 'emergency': System is unusable.
* 'alert': Action must be taken immediately.
* 'critical': Critical conditions.
* 'error': Error conditions.
* 'warning': Warning conditions.
* 'notice': Normal but significant condition.
* 'info': Informational messages.
* 'debug': Debug-level messages.
* @param string $message Log message.
* @param string $level One of the following:
* 'emergency': System is unusable.
* 'alert': Action must be taken immediately.
* 'critical': Critical conditions.
* 'error': Error conditions.
* 'warning': Warning conditions.
* 'notice': Normal but significant condition.
* 'info': Informational messages.
* 'debug': Debug-level messages.
* @param array<string, string> $context Context data.
*/
public function log( $message, $level = 'info' ): void {
public function log( $message, $level = 'info', $context = [] ): void {
if ( ! $this->can_log() ) {
return;
}
$this->wc_logger->log( $level, $message, [ 'source' => self::LOG_FILENAME ] );
$context = array_merge( $context, [ 'source' => self::LOG_FILENAME ] );
$this->wc_logger->log( $level, $message, $context );
}

/**
Expand Down
Loading
Loading