-
Notifications
You must be signed in to change notification settings - Fork 26
feat: implement logger #121
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
Conversation
q: why does the example you shared in #121 (comment) emit a textPayload not a jsonPayload as I'd expect when you give the log a structured data like a dictionary? |
@taeold The code was adapted from the node logger |
@exaby73 I'm not sure I understand what you mean. The purpose of nodejs logger is so that it's easy to emit structured log entry from valid JS object and not to emit them as just text message. e.g.
If the python logger isn't doing any work to generate structued logs using Can you confirm if:
|
@taeold I've updated the logger to support ![]() |
@@ -114,7 +114,7 @@ def _auth_user_record_from_token_data(token_data: dict[str, _typing.Any]): | |||
return AuthUserRecord( | |||
uid=token_data["uid"], | |||
email=token_data.get("email"), | |||
email_verified=token_data.get("email_verified"), | |||
email_verified=bool(token_data.get("email_verified")), |
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.
hmm this feels unrelated? should we make a separate PR for this or is this necessary for log impl?
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.
mypy was updated and was causing CI to fail because the types didn't match. I'm open to suggestions. This is mainly a linting fix
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.
maybe clearest to make the mypy related linting fixes in a separate PR, and have only the logging things here.
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.
Without these, the CI doesn't pass. Possibly can make a separate PR for these that can merged into main
, before we merge this in so that we can have those changes rebased into this branch. I'll look into doing this next week :)
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.
eh let's save ourselves some time and push it in this PR.
@@ -387,6 +387,8 @@ def timestamp_conversion(time: str) -> _dt.datetime: | |||
elif precision_timestamp == PrecisionTimestamp.SECONDS: | |||
return second_timestamp_conversion(time) | |||
|
|||
raise ValueError("Invalid timestamp") |
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.
likewise - related to log impl?
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.
Again a lint failure here because the function wasn't ending in a return and the if, elif is not exhaustive
@@ -114,7 +114,7 @@ def _auth_user_record_from_token_data(token_data: dict[str, _typing.Any]): | |||
return AuthUserRecord( | |||
uid=token_data["uid"], | |||
email=token_data.get("email"), | |||
email_verified=token_data.get("email_verified"), | |||
email_verified=bool(token_data.get("email_verified")), |
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.
eh let's save ourselves some time and push it in this PR.
@exaby73 thank you!! |
Closes #79