Remove optional import of newrelic

timmc-edx commented 7 months ago

edx-django-utils.monitoring currently does a try-import of the newrelic package and then sets a flag if the import fails. This is a historical artifact that is no longer needed; we can just import newrelic.agent directly and drop any if newrelic: checks.

Archaeological notes:

edx-django-utils already has a base requirement on the newrelic package, and has from the very beginning.
The monitoring code was originally copied from edx-platform in 2018: https://github.com/openedx/edx-django-utils/pull/2
In 2017, there was a PR in edx-platform to perform this conditional calling of newrelic code: https://github.com/openedx/edx-platform/pull/14633
The reason to not assume the newrelic dependency was loaded was because it needed to be removed from the base requirements of edx-platform due to having a proprietary license.
However, ever since newrelic==5.16.0.145 in 2020, the package has been under the Apache license.

robrap commented 7 months ago

Thanks for the history.

If a deployment had removed this dependency, even for an outdated reason, this would cause a backward-incompatibility. I don’t think we should change anything around this until this code is no longer run by default in the first place.

timmc-edx commented 7 months ago

I think someone would have to go to some lengths to avoid having it installed, as it's currently pulled in automatically. I don't think it would be a big deal to do a major-version bump and remove the conditionals. But it's certainly not urgent; the main reason I filed this ticket was just to have a place to link to in a comment. :-)

robrap commented 6 months ago

@timmc-edx will look at this to determine whether to backlog or close.

timmc-edx commented 6 months ago

I'm going to go ahead and close this for now. With the work in https://github.com/openedx/edx-django-utils/pull/397 we might be moving towards a situation where we can contain all of the New Relic references to a single plugin, and at that point it might be relatively straightforward to move newrelic from base to testing dependencies. But it's too early to say at this point.

openedx / edx-django-utils

Remove optional import of newrelic #396