Skip to content
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

Merged
merged 19 commits into from
Nov 11, 2021
Merged

Conversation

daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Oct 14, 2021

This PR adds a number of small user experience fixes, mostly centered around making the API more forgiving to imperfect inputs

  • added support to duplicate certain fields out of logged structs and into the paylolad (eg, severity)
  • severity values will be auto-converted to upper-case, so they can be parsed properly into enum form
  • added tests to ensure pandas dataframes could be logged directly without causing a crash
  • if users enter a resource in dict format instead of as a full Resource object, attempt to parse it into an object instead of failing

@product-auto-label product-auto-label bot added the api: logging Issues related to the googleapis/python-logging API. label Oct 14, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 14, 2021
@daniel-sanche daniel-sanche changed the title [DRAFT] feat: make logging API more friendly to use feat: make logging API more friendly to use Oct 15, 2021
@daniel-sanche daniel-sanche marked this pull request as ready for review October 15, 2021 00:15
@daniel-sanche daniel-sanche requested review from a team as code owners October 15, 2021 00:15
Comment on lines 146 to 150
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}")
Copy link
Contributor

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:

Suggested change
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"])

Copy link
Contributor Author

@daniel-sanche daniel-sanche Oct 15, 2021

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

Copy link
Contributor

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.

Copy link
Contributor

@losalex losalex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@minherz minherz left a 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
Copy link
Contributor

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?

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.

Copy link
Contributor Author

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

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.

Comment on lines 384 to 385
if _STRUCT_EXTRACTABLE_FIELDS are present in the struct and not given,
they should be used in the LogEntry

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

Copy link
Contributor Author

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

@daniel-sanche
Copy link
Contributor Author

nit: if you keep code raising exception if the dictionary is not well formed, it worth adding a test for it as well

I did add some system tests around this, but if you think you see any gaps, let me know

@daniel-sanche daniel-sanche added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 26, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the googleapis/python-logging API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants