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.

Sign up for GitHub

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

Jump to bottom

FIX: resilient environment settings #825

Merged
merged 8 commits into from
Jun 27, 2023
Merged

FIX: resilient environment settings #825

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a298b27
FIX: resilient environment settings
aaeabdo Jun 6, 2023
5775452
Merge branch 'main' into resilient-environment-settings
mergify[bot] Jun 12, 2023
ab7a9cd
Merge branch 'main' into resilient-environment-settings
mergify[bot] Jun 12, 2023
98bf646
Merge branch 'main' into resilient-environment-settings
mergify[bot] Jun 14, 2023
7cac62c
Merge branch 'main' into resilient-environment-settings
hmstepanek Jun 21, 2023
bfaf52a
Merge branch 'main' into resilient-environment-settings
hmstepanek Jun 21, 2023
9f9b13a
Merge branch 'main' into resilient-environment-settings
hmstepanek Jun 27, 2023
84cdb08
Merge branch 'main' into resilient-environment-settings
mergify[bot] Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion newrelic/core/environment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,15 @@ def environment_settings():

# If the module isn't actually loaded (such as failed relative imports
# in Python 2.7), the module will be None and should not be reported.
if not module:
try:
if not module:
continue
except Exception:
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something really obvious here but...I don't really understand this exception block. We are just grabbing the module out of sys.modules here so it's not being import inside the try block as far as I understand so how would this exception get raised in this case? It seems like adding the content inside the try block would be enough to handle the module failing to load.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, you're correct; it is indeed confusing. I am currently developing an application that relies on a package using the 'generalimport' package. This package is used to import optional dependencies without having to install them. It raises an exception if the code attempts to call an optional dependency that hasn't been installed. In some way, 'if not module' is interpreted as if the code is trying to use the module, leading to the 'missing dependency' exception. I suspect the flaw lies in the 'generalimport' package. However, I believe the agent should be resilient enough not to fail when building environment settings. Catching the exception here prevents the agent from crashing. Does this make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for that explanation! Ideally we would have a test implemented for this. Let me get back to you on how we might go about setting up the env for that! I don't think we'd want to just add generalimport in as a dependency into the existing env cause then we'd be using it in all our tests. I think we'd instead want to make another env and only run through a couple tests but I'll consult with my team and see what they think would be best.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I've talked to my team! As far as testing generalimport; we decided we're just going to manually test it for now which I've already done and this looks good to me! For the python2.7 failed relative imports; let's add a new test for that into https://github.com/newrelic/newrelic-python-agent/blob/main/tests/agent_unittests/test_environment.py. You could probably copy-modify test_plugin_list and maybe call it something like test_failed_relative_imports_no_harm.

Copy link
Contributor

Choose a reason for hiding this comment

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

And...I just realized that python 2.7 logic was there already (sorry about that!) so never mind on adding a test for that! I'm gonna go ahead and run the tests for this and merge it if it looks ok in CI.

# if the application uses generalimport to manage optional depedencies,
# it's possible that generalimport.MissingOptionalDependency is raised.
# In this case, we should not report the module as it is not actually loaded and
# is not a runtime dependency of the application.
#
continue

# Exclude standard library/built-in modules.
Expand Down