feat(logging): GoogleSdkLoggerDelegator initial implementation#49
feat(logging): GoogleSdkLoggerDelegator initial implementation#49cy-yun wants to merge 1 commit into
Conversation
|
These two pull requests introduce a standardized and dynamically configurable debug logging framework for Google Cloud Ruby clients and implement it in the Pub/Sub library. The primary goal was to enable on-demand debug logging that could be controlled by the developer via configuring a custom logger and controlling an environment variable to turn logs on/off. This required passing a logger from the handwritten client (google-cloud-pubsub) down to the underlying GAPIC client (google-cloud-pubsub-v1). I needed to build a wrapper around the logger (creating a logger duck-type) that will evaluate the environment variable before emitting logs. The Solution:
Once the first 2 PRs are approved, I will create a new PR that passes the GoogleSdkLoggerDelegator down to the GAPIC layer (google-cloud-pubsub-v1). |
viacheslav-rostovtsev
left a comment
viacheslav-rostovtsev
left a comment
There was a problem hiding this comment.
I think this approach is fine. A reasonable alternative would be to require a wrapped logger to handle its errors (like Logger does) and trust that whatever errors are going to be raised from the underlying logger are the errors that the end-user wants to see.
| # A delegator that wraps a logger or its duck-type instance to add dynamic | ||
| # filtering based on GOOGLE_SDK_RUBY_LOGGING_GEMS environment variable, and | ||
| # error supression based on suppress_logger_errors parameter | ||
| class GoogleSdkLoggerDelegator < SimpleDelegator |
There was a problem hiding this comment.
Consider calling it "Proxy" or "Wrapper" instead of "Delegator".
| class GoogleSdkLoggerDelegator < SimpleDelegator | ||
| # @private | ||
| # The environment variable that controls which gems have logging enabled. | ||
| ENV_VAR = "GOOGLE_SDK_RUBY_LOGGING_GEMS".freeze |
There was a problem hiding this comment.
"LOGGING_GEMS_ENV_VAR"
| suppress_errors { super } | ||
| end | ||
|
|
||
| def debug message = nil, &block |
There was a problem hiding this comment.
since it's Ruby 3.1 we can probably drop the block variable name and just use &
|
|
||
| # Return the cached result if the ENV var hasn't changed. | ||
| if @cached_env_var_value == current_env_var | ||
| return @cached_logging_enabled_result |
There was a problem hiding this comment.
cached instance-level variables should be explicitly initialized in the constructor.
| def is_logger_type? logger | ||
| return false if logger.nil? | ||
| REQUIRED_LOGGER_METHODS.all? { |m| logger.respond_to? m } | ||
| end |
There was a problem hiding this comment.
This looks like a class-level (static) method
|
|
||
| def << msg | ||
| return true unless logging_enabled? | ||
| suppress_errors { super } |
There was a problem hiding this comment.
something like handle_logging_errors would be a better name, similar to handle_writing_errors in Logger's LogDevice
| # @param gem_name [String] The name of the gem this logger is for. | ||
| # @param logger [Logger] The custom logger instance to wrap. | ||
| # @param suppress_logger_errors [Boolean] Whether to swallow exceptions in the wrapped logger. | ||
| def initialize gem_name, logger, suppress_logger_errors: true |
There was a problem hiding this comment.
I wonder if we should make it same as Logger by giving it a list of Errors to reraise instead of a blanket variable.
| require "google/logging/message" | ||
| require "google/logging/source_location" | ||
| require "google/logging/structured_formatter" | ||
| require "google/logging/google_sdk_filtered_logger" |
3 participants
A delegator that wraps a logger or its duck-type instance to add dynamic filtering based on GOOGLE_SDK_RUBY_LOGGING_GEMS environment variable, and error supression based on suppress_logger_errors parameter. To be used for client library & GAPIC layer logging.