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

Patched _proxy_log to be more useful #110

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

j0yu
Copy link

@j0yu j0yu commented May 14, 2020

Problem

If proxy messages fail to log, the exception message logged does very little to help debug erroneous message formatting i.e. typically just be something like:

TypeError: not all arguments converted during string formatting
# or
TypeError: not enough arguments for format string

which unfortunately, is find for where error occured but terrible for figuring out how or what caused the error without jumping into a debugger to figure out the exact values of msg or *args:

  1. When msg parameter in is not a string and gets erroneously expanded

    >>> msg = ("hello", "there")
    >>> "[PROXY] %s" % msg
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: not all arguments converted during string formatting
  2. Bad amounts of *args passed in for the final, "PROXY" pre-pended msg

    >>> import logging
    
    >>> logging.warn("[PROXY] %s", "more", "than", "1")
    Traceback (most recent call last):
      File "/usr/lib64/python2.7/logging/__init__.py", line 851, in emit
        msg = self.format(record)
      File "/usr/lib64/python2.7/logging/__init__.py", line 724, in format
        return fmt.format(record)
      File "/usr/lib64/python2.7/logging/__init__.py", line 464, in format
        record.message = record.getMessage()
      File "/usr/lib64/python2.7/logging/__init__.py", line 328, in getMessage
        msg = msg % self.args
    TypeError: not all arguments converted during string formatting
    Logged from file <stdin>, line 1
    
    >>> logging.warn("[PROXY] %s %s %s %s", "not enough")
    Traceback (most recent call last):
      File "/usr/lib64/python2.7/logging/__init__.py", line 851, in emit
        msg = self.format(record)
      File "/usr/lib64/python2.7/logging/__init__.py", line 724, in format
        return fmt.format(record)
      File "/usr/lib64/python2.7/logging/__init__.py", line 464, in format
        record.message = record.getMessage()
      File "/usr/lib64/python2.7/logging/__init__.py", line 328, in getMessage
        msg = msg % self.args
    TypeError: not enough arguments for format string
    Logged from file <stdin>, line 1

Because these are proxy messages, they tend to hide which Shotgun app/engine/framework is causing the error.

Changed

  • Report level, message and arguments when logging proxy message log failures

Fixed

  • Prepend [PROXY] using str.format instead of % to avoid potential expansion of msg
  • args to be *args so when single, iterable argument is passed in, it doesn't get unnecessarily expanded e.g. a dict

@jfboismenu
Copy link
Contributor

Hi,
Could you give an example of a situation where this muting of the error caused issues? I think this fix was introduced originally because an error was raised by the logger because the inputs weren't valid. Trying to serialize the parameters would likely rethrow the error.

@j0yu
Copy link
Author

j0yu commented Jun 3, 2020

Sure! This patch is basically to make error messages more easy to debug bad msg or *args, which would typically just be something like:

TypeError: not all arguments converted during string formatting
# or
TypeError: not enough arguments for format string

which unfortunately, is find for where error occured but terrible for figuring out how or what caused the error without jumping into a debugger to figure out the exact values of msg or *args:

  1. When msg parameter in is not a string and gets erroneously expanded

    >>> msg = ("hello", "there")
    >>> "[PROXY] %s" % msg
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: not all arguments converted during string formatting
  2. Bad amounts of *args passed in for the final, "PROXY" pre-pended msg

    >>> import logging
    
    >>> logging.warn("[PROXY] %s", "more", "than", "1")
    Traceback (most recent call last):
      File "/usr/lib64/python2.7/logging/__init__.py", line 851, in emit
        msg = self.format(record)
      File "/usr/lib64/python2.7/logging/__init__.py", line 724, in format
        return fmt.format(record)
      File "/usr/lib64/python2.7/logging/__init__.py", line 464, in format
        record.message = record.getMessage()
      File "/usr/lib64/python2.7/logging/__init__.py", line 328, in getMessage
        msg = msg % self.args
    TypeError: not all arguments converted during string formatting
    Logged from file <stdin>, line 1
    
    >>> logging.warn("[PROXY] %s %s %s %s", "not enough")
    Traceback (most recent call last):
      File "/usr/lib64/python2.7/logging/__init__.py", line 851, in emit
        msg = self.format(record)
      File "/usr/lib64/python2.7/logging/__init__.py", line 724, in format
        return fmt.format(record)
      File "/usr/lib64/python2.7/logging/__init__.py", line 464, in format
        record.message = record.getMessage()
      File "/usr/lib64/python2.7/logging/__init__.py", line 328, in getMessage
        msg = msg % self.args
    TypeError: not enough arguments for format string
    Logged from file <stdin>, line 1

j0yu pushed a commit to wwfxuk/tk-desktop that referenced this pull request Jun 3, 2020
@j0yu
Copy link
Author

j0yu commented Jun 5, 2020

Because these are proxy messages, they tend to hide which Shotgun app/engine/framework is causing the error.

I guess you could also just add the full stack trace to after except Exception:, it would work too as you can the files leading up to the error but it might be fairly long and still won't show the values of the arguments that contributes the error

@j0yu j0yu force-pushed the meaningful-proxy-log-error branch from 72307e7 to bec530f Compare August 13, 2020 09:39
j0yu pushed a commit to wwfxuk/tk-desktop that referenced this pull request Nov 5, 2020
@j0yu j0yu force-pushed the meaningful-proxy-log-error branch from bec530f to 3f4c2b1 Compare November 5, 2020 14:03
@Liametc Liametc force-pushed the meaningful-proxy-log-error branch from 3f4c2b1 to 66be354 Compare March 11, 2021 15:59
@j0yu j0yu force-pushed the meaningful-proxy-log-error branch from 66be354 to 3fa2524 Compare June 8, 2021 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants