-
Notifications
You must be signed in to change notification settings - Fork 112
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
Addresses Issue #245 for better logging #332
base: main
Are you sure you want to change the base?
Addresses Issue #245 for better logging #332
Conversation
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.
Thanks @patilvishal0597, looks good overall. Added a few minor comments.
import os | ||
|
||
# Environment variable to set the application logger(s) in debug mode during runtime | ||
__DEBUG = True if os.environ.get("LANGCHAIN_AWS_DEBUG") else 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.
os.environ.get()
returns a string - this line will set __DEBUG
to True
in any case where LANGCHAIN_AWS_DEBUG
has been defined. So we might see unexpected behavior if the user sets something like LANGCHAIN_AWS_DEBUG=false
.
An explicit string comparison like this would work better:
__DEBUG = os.getenv("LANGCHAIN_AWS_DEBUG", "").lower() in ["true", "1"]
# Flag for root debug logger to set the root debug logger in debug mode during runtime | ||
# Root debug logger will print boto3 as well as application debug logs if set to true | ||
# This flag will be set to true if LANGCHAIN_AWS_DEBUG = LANGCHAIN_AWS_DEBUG_ROOT = true | ||
__ROOT_DEBUG = __DEBUG if os.environ.get("LANGCHAIN_AWS_DEBUG_ROOT") else 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.
Same as for __DEBUG
on Line 5.
# else ERROR | ||
if __DEBUG: | ||
DEFAULT_LOG_LEVEL: int = logging.DEBUG | ||
else: | ||
DEFAULT_LOG_LEVEL: int = logging.INFO |
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.
The comment says ERROR
, but conditional uses INFO
level.
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.
Changing the default log level to "ERROR"
DEFAULT_LOG_FILE = os.environ.get("LANGCHAIN_AWS_LOG_OUTPUT", "-") | ||
if DEFAULT_LOG_FILE == "-": |
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.
Is there a reason for overriding with -
here? Can we use the os.environ.get()
default value so that the conditional doesn't need to check for an explicit value?
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've added -
as a default value in case the DEFAULT_LOG_FILE
value isn't set. Do we want to have a default value as None
to make this cleaner?
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.
It's unnecessary to set None
explicitly - os.environ.get
already returns None
by default if the variable doesn't exist.
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.
fixed in the new commit
55aeab9
to
9eb0ea3
Compare
9eb0ea3
to
c3768d5
Compare
|
||
# Environment variable to set the application logger(s) in debug mode during runtime | ||
LANGCHAIN_AWS_DEBUG: str = os.environ.get("LANGCHAIN_AWS_DEBUG", "false") | ||
__DEBUG: bool = True if LANGCHAIN_AWS_DEBUG.lower() in ["true", "1"] else 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.
Nit: simplify to
__DEBUG: bool = LANGCHAIN_AWS_DEBUG.lower() in ["true", "1"]
except ImportError: | ||
colorama = None | ||
coloredlogs = None | ||
|
||
DEFAULT_LOG_FORMATTER: logging.Formatter = logging.Formatter(DEFAULT_LOG_FORMAT) |
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.
IMO we shouldn't fail this silently - add an logger statement in case users are interested in enabling coloring
c3768d5
to
694f93f
Compare
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.
LGTM - thanks @patilvishal0597 !
Added `logger_util` to enable package and class wide logging in `langchain-aws`. Added logging for `invoke` and `ainvoke`. langchain-ai#245
4c98e33
to
9c2432e
Compare
Added
logger_util
to enable package and class wide logging inlangchain-aws
. Added logging forinvoke
andainvoke
.Added a logger util for configuration of logger. Tested logging of invoke, ainvoke, and converse request/responses
Behaviour:
LANGCHAIN_AWS_DEBUG = LANGCHAIN_AWS_DEBUG_ROOT = True --> Logs debug messages across boto3 and application
LANGCHAIN_AWS_DEBUG = True; LANGCHAIN_AWS_DEBUG_ROOT = True --> Logs only applcation debug messages
LANGCHAIN_AWS_DEBUG = False; LANGCHAIN_AWS_DEBUG_ROOT = True --> Logs only application info messages
LANGCHAIN_AWS_DEBUG = LANGCHAIN_AWS_DEBUG_ROOT = False --> Logs only application info messages
Invoke call:
Logging output:
Debug logs with LANGCHAIN_AWS_DEBUG and LANGCHAIN_AWS_DEBUG_ROOT env vars as True
Logging output when LANGCHAIN_AWS_DEBUG and LANGCHAIN_AWS_DEBUG_ROOT flags are False and logger is initialized with a module_name: