-
Notifications
You must be signed in to change notification settings - Fork 55
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 logging API more friendly to use #422
Conversation
google/cloud/logging_v2/logger.py
Outdated
try: | ||
kw["resource"] = Resource(**kw["resource"]) | ||
except TypeError as e: | ||
# dict couldn't be parsed as a Resource | ||
raise TypeError(f"invalid resource dict. {e}") |
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.
Rebinding the exception loses information. The default traceback should indicate where the failure occurred, and should be fine for the developer to debug:
try: | |
kw["resource"] = Resource(**kw["resource"]) | |
except TypeError as e: | |
# dict couldn't be parsed as a Resource | |
raise TypeError(f"invalid resource dict. {e}") | |
kw["resource"] = Resource(**kw["resource"]) |
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 problem is the raw error message doesn't make it clear that the resource is the issue:
TypeError: __new__() missing 2 required positional arguments: 'type' and 'labels'
That's a good point that we shouldn't lose the context with a fresh exception though. What do you think about following this pattern, to add the extra context on top of the original exception?
try:
kw["resource"] = Resource(**kw["resource"])
except TypeError as e:
raise TypeError("invalid resource dict") from e
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 guess I was expecting that seeing a traceback with the Resource(**kw["resource"])
call at the point of exception was a pretty good clue. If it still needs more, then raise .. from
is certainly the Right Thing(TM) to preserve the original traceback.
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
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: if you keep code raising exception if the dictionary is not well formed, it worth adding a test for it as well
kw["resource"] = Resource(**kw["resource"]) | ||
except TypeError as e: | ||
# dict couldn't be parsed as a Resource | ||
raise TypeError("invalid resource dict") from e |
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.
does it mean the writing log fails? is it a current user experience in the case of other failures?
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.
+1 to the question. We generally should try to avoid crashing the library.
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.
Before this change, the library would already throw the same exception here for the same inputs. This change attempts to turn certain error-raising inputs into valid inputs. But some inputs will still be invalid
Also, note that exceptions are used for control flow in Python, so raising an exception doesn't necessarily mean "crash"
kw["resource"] = Resource(**kw["resource"]) | ||
except TypeError as e: | ||
# dict couldn't be parsed as a Resource | ||
raise TypeError("invalid resource dict") from e |
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.
+1 to the question. We generally should try to avoid crashing the library.
tests/unit/test_logger.py
Outdated
if _STRUCT_EXTRACTABLE_FIELDS are present in the struct and not given, | ||
they should be used in the LogEntry |
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.
can you rephrase this? It's a bit confusing
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 tried to fix it, but it may still be a bit hard to parse. Let me know if you have a suggestion
I did add some system tests around this, but if you think you see any gaps, let me know |
This PR adds a number of small user experience fixes, mostly centered around making the API more forgiving to imperfect inputs
Resource
object, attempt to parse it into an object instead of failing