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

Fixed python 3 incompatibility in logging API implemenation of findCaller (v3.3 cherry pick) #5059

Merged
merged 5 commits into from
Oct 15, 2020

Conversation

nmaludy
Copy link
Member

@nmaludy nmaludy commented Oct 14, 2020

Closes #5057

See that issue for description of what the problem was. The fix was simply to ensure our implementation handles both the python 2 version and returns a 3-tuple, along with the python 3 version which returns a 4-tuple and is based on the code here: https://github.com/python/cpython/blob/3.9/Lib/logging/__init__.py#L1502-L1536

I've tested this change both my a python 2.7 (centos7) and python 3.6 (centos8 and ubuntu18) box and the logging modules are returned properly.

@nmaludy nmaludy added the bug label Oct 14, 2020
@nmaludy nmaludy added this to the 3.3.0 milestone Oct 14, 2020
@nmaludy nmaludy requested review from blag, punkrokk and arm4b October 14, 2020 20:30
@nmaludy nmaludy self-assigned this Oct 14, 2020
@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Oct 14, 2020
@nmaludy nmaludy requested a review from amanda11 October 14, 2020 20:40
and expects a 4-element tuple to be returned, rather than a 3-element tuple in
the python 2 implementation.
We derived our implementation from the Python 3.9 source code here:
https://github.com/python/cpython/blob/3.9/Lib/logging/__init__.py#L1376-L1404
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
https://github.com/python/cpython/blob/3.9/Lib/logging/__init__.py#L1376-L1404
https://github.com/python/cpython/blob/3.9/Lib/logging/__init__.py#L1502-L1536

@amanda11
Copy link
Contributor

I installed the package from this PR onto U18 and it did get rid of most of the unknown file messages in the logs. I'm still left with "unknown file" on exception in the st2sensorcontainer log:

2020-10-15 08:22:43,653 139699041297216 ERROR (unknown file) [-] File "/opt/stackstorm/packs/openstack/sensors/messaging_sensor.py", line 52, in poll

@nmaludy
Copy link
Member Author

nmaludy commented Oct 15, 2020

@amanda11 can you confirm if that is a new problem or one that already exists in 3.2?

@amanda11
Copy link
Contributor

@amanda11 can you confirm if that is a new problem or one that already exists in 3.2?

I think it probably exists on 3.2 as 3.2 had lots of "unknown file" messages, but I'll confirm - I have to break a system to get the exception!

@amanda11
Copy link
Contributor

@amanda11 can you confirm if that is a new problem or one that already exists in 3.2?

@nmaludy - confirmed it is python 3 only. I can raise an issue for it - it just seems to be the exceptions that are a problem now on 3.3.

Copy link
Contributor

@amanda11 amanda11 left a comment

Choose a reason for hiding this comment

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

LGTM - I willl raise a separate issue for the "unknown module" on exceptions in sensor container on py3 (if one already doesn't exist), after confirming that the problem also exists on 3.2.

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

LGTM

@nmaludy nmaludy merged commit 1689445 into v3.3 Oct 15, 2020
@nmaludy nmaludy deleted the hotfix/python3-logging-filename branch October 15, 2020 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug size/M PR that changes 30-99 lines. Good size to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants