fix: Use edx-django-utils API instead of calling newrelic directly#476
fix: Use edx-django-utils API instead of calling newrelic directly#476sarina merged 2 commits intoopenedx:masterfrom
Conversation
|
@timmc-edx , could you point (either in comment here or in code) to the right way to fix this in the long term? |
|
Instead of I was just going for the least-work option, but it does look like edx-django-utils is already in the IDA's requirements, so maybe it would be better to just go ahead and make that change. Either way works for me, I'm just clearing the path for a DEPR. :-) |
timmc-edx
force-pushed
the
timmc/no-direct-nr
branch
from
4a08ad1 to
345c483
Compare
timmc-edx
changed the title
|
I went back and made the better fix -- PR name and description updated, too. |
We should remove newrelic references entirely and replace them with edx-django-utils calls. This change just unblocks removing newrelic as a direct dependency.
These changes will cause telemetry to start being reported when annotations are created. (No change for whether notesserver heartbeat calls emit telemetry, as that is currently only supported for New Relic anyhow.) This also removes the newrelic dependency, though it is still pulled in by edx-django-utils indirectly, for now.
timmc-edx
force-pushed
the
timmc/no-direct-nr
branch
from
345c483 to
b037599
Compare
| @@ -109,9 +109,7 @@ meilisearch==0.34.0 | |||
| mysqlclient==2.2.7 | |||
| # via -r requirements/base.in | |||
| newrelic==10.7.0 | |||
There was a problem hiding this comment.
Looks like newrelic is still included in the requirements, is that intentional?
There was a problem hiding this comment.
Yes -- if you look at the comments below that line, you can see that it's still being pulled in via edx-django-utils (for now).
There was a problem hiding this comment.
What's the future plan for removing it from requirements?
There was a problem hiding this comment.
As far as I can tell, this was the last instance in the edx and openedx orgs where we still had a direct import newrelic -- so after this merges, I think we're clear to remove newrelic from the edx-django-utils base.in as well. I don't have a specific plan for that, but I did want to clear the path for it.
There was a problem hiding this comment.
Update: Filed a DEPR for this: openedx/public-engineering#360
4 participants
These changes will cause telemetry to start being reported when annotations are created. (No change for whether notesserver heartbeat calls emit telemetry, as that is currently only supported for New Relic anyhow.)
This also removes the newrelic dependency, though it is still pulled in by edx-django-utils indirectly, for now.
[EDIT: This originally just wrapped the newrelic import in a try block, but the new PR removes newrelic references entirely.]