-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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: make log format configurable #4851
Conversation
CodSpeed Performance ReportMerging #4851 will degrade performances by 27.98%Comparing Summary
Benchmarks breakdown
|
Would it be better to use imho: Setting the format directly will give the user more flexibility and is less implicit than the log environments but comes with the downside of configuration errors in the logging which will make diagnostics pretty hard. If the maintainers decide that |
Hey @Kraego How are you? This is a valid problem. If we were to use LANGFLOW_ENV we'd have to build something like the following to check for valid format strings: import logging
def is_valid_log_format(format_string):
"""Validates a logging format string by attempting to format it with a dummy LogRecord.
Args:
format_string (str): The format string to validate.
Returns:
bool: True if the format string is valid, False otherwise.
str: Error message if invalid, empty string if valid.
"""
record = logging.LogRecord(
name="dummy",
level=logging.INFO,
pathname="dummy_path",
lineno=0,
msg="dummy message",
args=None,
exc_info=None
)
formatter = logging.Formatter(format_string)
try:
# Attempt to format the record
formatter.format(record)
except (KeyError, ValueError, TypeError) as e:
# Formatting failed, return False and the error message
msg = f"Invalid log format string: {format_string}"
raise ValueError(msg) from e
return True What do you think? |
Hi @ogabrielluiz, thanks I am doing well hope you too ... As far as I see:
Both Options above will resolve my problem. What option should I implement? |
I've added your proposed format string check |
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 @Kraego !
LGTM
@ogabrielluiz now ready for merge |
Hey @Kraego Sounds good! I'll merge it ASAP. You can leave it out of date because it will only merge when no other PR is merging. |
Since I had problems with the encoding of the colored json log in the OKD shell (see Image), I've made the logformat configurable.