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

Improve resiliency of logging initialization phase #3705

Merged
merged 5 commits into from
May 3, 2019
Merged

Conversation

masci
Copy link
Contributor

@masci masci commented May 2, 2019

What does this PR do?

  1. encode the string when in Python2 and the input is unicode (this will be the normal case in Agent 6.12)
  2. assume the input can be bogus since parameter is read from a config file
  3. make the fallback case consistent (we now have 2 different defaults)

Review checklist (to be filled by reviewers)

  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • Feature or bugfix must have tests
  • Git history must be clean
  • If PR adds a configuration option, it must be added to the configuration file.

if PY2 and isinstance(lvl, text_type):
try:
lvl = lvl.encode('utf-8')
except UnicodeError:
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to catch that, we may as well pass errors="ignore" to the encode call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, even better at this point would be using ascii

# default value for invalid input
assert log._get_py_loglevel(None) == logging.INFO
# default value for invalid unicode input
assert log._get_py_loglevel(u'dèbùg') == logging.INFO
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK this is valid utf-8 input, so it's encoded properly. I guess we need so weirder string to test it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the intention was to test a proper encoding but into a key that's invalid

@codecov
Copy link

codecov bot commented May 2, 2019

Codecov Report

Merging #3705 into master will decrease coverage by 2.45%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3705      +/-   ##
==========================================
- Coverage   86.42%   83.96%   -2.46%     
==========================================
  Files         730       62     -668     
  Lines       37041     4702   -32339     
  Branches     4360      574    -3786     
==========================================
- Hits        32012     3948   -28064     
+ Misses       3855      633    -3222     
+ Partials     1174      121    -1053

Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

💯

@masci masci merged commit 3309de9 into master May 3, 2019
@masci masci deleted the massi/loginit branch May 3, 2019 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants