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

pyramid: Fix which package is the correct caller in _traced_init. #830

Merged
merged 19 commits into from
Apr 14, 2022

Conversation

kenrobbins
Copy link
Contributor

@kenrobbins kenrobbins commented Dec 9, 2021

Description

The _traced_init wrapper is getting one caller too far up. Example below.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I put a pdb statement in the Registry constructor, which is called by the Configurator constructor. This is the same code running. The top has otel enabled and the bottom has it disabled.

  /home/ken/.pyenv/versions/3.7.5/envs/webapp/lib/python3.7/site-packages/paste/deploy/loadwsgi.py(152)invoke()
-> return fix_call(context.object, context.global_conf, **context.local_conf)
  /home/ken/.pyenv/versions/3.7.5/envs/webapp/lib/python3.7/site-packages/paste/deploy/util.py(55)fix_call()
-> val = callable(*args, **kw)
  /opt/src/webapp/webapp/__init__.py(44)main()
-> config = Configurator(root_factory=Root, settings=settings)
  /home/ken/.pyenv/versions/3.7.5/envs/webapp/lib/python3.7/site-packages/opentelemetry/instrumentation/pyramid/__init__.py(142)_traced_init()
-> wrapped(*args, **kwargs)
  /home/ken/.pyenv/versions/3.7.5/envs/webapp/lib/python3.7/site-packages/pyramid/config/__init__.py(314)__init__()
-> registry = Registry(self.package_name)
> /home/ken/.pyenv/versions/3.7.5/envs/webapp/lib/python3.7/site-packages/pyramid/registry.py(62)__init__()
-> Components.__init__(self, package_name, *args, **kw)
(Pdb) package_name
'paste.deploy'

 /home/ken/.pyenv/versions/3.7.5/envs/webapp/lib/python3.7/site-packages/paste/deploy/loadwsgi.py(152)invoke()
-> return fix_call(context.object, context.global_conf, **context.local_conf)
  /home/ken/.pyenv/versions/3.7.5/envs/webapp/lib/python3.7/site-packages/paste/deploy/util.py(55)fix_call()
-> val = callable(*args, **kw)
  /opt/src/webapp/webapp/__init__.py(44)main()
-> config = Configurator(root_factory=Root, settings=settings)
  /home/ken/.pyenv/versions/3.7.5/envs/webapp/lib/python3.7/site-packages/pyramid/config/__init__.py(314)__init__()
-> registry = Registry(self.package_name)
> /home/ken/.pyenv/versions/3.7.5/envs/webapp/lib/python3.7/site-packages/pyramid/registry.py(62)__init__()
-> Components.__init__(self, package_name, *args, **kw)
(Pdb) package_name
'webapp'

Added unit test to ensure registry name value matches the module of the caller.

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@kenrobbins kenrobbins requested a review from a team December 9, 2021 02:02
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 9, 2021

CLA Signed

The committers are authorized under a signed CLA.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @kenrobbins! Please sign the CLA

CHANGELOG.md Outdated Show resolved Hide resolved
@kenrobbins
Copy link
Contributor Author

CLA is signed. Tests added. Should be mergeable now I think.

@ocelotl
Copy link
Contributor

ocelotl commented Mar 17, 2022

@kenrobbins please fix lint ✌️

Fix quotes in pyramid test
@srikanthccv
Copy link
Member

@kenrobbins Please fix the lint and conflict

@kenrobbins
Copy link
Contributor Author

@ocelotl @srikanthccv please check now. should be fixed. thanks.

@srikanthccv
Copy link
Member

@kenrobbins requires some re-formatting to fix lint and we are good to go.

@kenrobbins
Copy link
Contributor Author

@ocelotl @srikanthccv fixed lint again. 🤞 thanks.

@lzchen lzchen closed this Apr 4, 2022
@lzchen lzchen reopened this Apr 4, 2022
@lzchen lzchen closed this Apr 4, 2022
@lzchen lzchen reopened this Apr 4, 2022
@lzchen
Copy link
Contributor

lzchen commented Apr 6, 2022

@kenrobbins
Looks like there is a test that is failing?

@kevellis124
Copy link

kevellis124 commented Apr 14, 2022

So I've been trying to see what's going on with the new test that is failing.
@lzchen it looks like the actual _trace_init func for pyramid was perhaps designed to be run on pypy and not CPython
Looks like the assumption in the code is that the caller_package needs to look a certain amount of frames up to get the caller. It defaults to 2 which works for CPython implementations of Python, but when using Pypy, there is another Wrapt frame which requires looking above another frame.

Current working theory:
Current code is broken for CPython but works for Pypy

Kens fix fixes it for CPython but breaks it for Pypy

Possible fix: conditionally check for which interpreter is being used in _traced_init to decide how many arbitrary frames we need to look through

@kevellis124
Copy link

import sys
if "PyPy" in sys.version:
  level=3
else:
  level=2

@kenrobbins
Copy link
Contributor Author

I committed a change to account for that. Let's see if it passes.

@kevellis124
Copy link

kevellis124 commented Apr 14, 2022

@srikanthccv if you have a second to approve the workflows again we'd appreciate it 🙏
Once more please

@kevellis124
Copy link

Once more please @srikanthccv 😬 thanks

@srikanthccv srikanthccv merged commit 4ad7592 into open-telemetry:main Apr 14, 2022
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.

7 participants