FIX: resilient environment settings

[aaeabdo]

Overview

if the application uses generalimport to manage optional dependencies, 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.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ aaeabdo
✅ hmstepanek
❌ mergify[bot]
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[hmstepanek]

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.

[aaeabdo]

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?

[hmstepanek]

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.

[hmstepanek]

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.

[hmstepanek]

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.

[hmstepanek]

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.

[codecov-commenter]

Codecov Report

Merging #825 (9f9b13a) into main (ab590a2) will decrease coverage by 0.01%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main     #825      +/-   ##
==========================================
- Coverage   76.41%   76.41%   -0.01%     
==========================================
  Files         196      196              
  Lines       19325    19328       +3     
  Branches     3310     3310              
==========================================
+ Hits        14767    14769       +2     
- Misses       3706     3708       +2     
+ Partials      852      851       -1     

Impacted Files	Coverage Δ
newrelic/core/environment.py	63.33% <75.00%> (-0.62%)	⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[hmstepanek]

Hey @aaeabdo - could you sign the Contributor License Agreement real quick? Then I can merge this in.

[aaeabdo]

Hey @aaeabdo - could you sign the Contributor License Agreement real quick? Then I can merge this in.

Hey @hmstepanek, I did earlier #825 (comment) - is this another one?

[hmstepanek]

@aaeabdo Ah probably just a timing issue with CI. Rebasing to the latest and re-running the tests.

Hey @aaeabdo - could you sign the Contributor License Agreement real quick? Then I can merge this in.

Hey @hmstepanek, I did earlier #825 (comment) - is this another one?

Oh! Probably just a timing issue between when you signed it and when the CI check ran then. I'm updating your branch to the latest base and re-running CI checks now...