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

feat: updated new relic reporting to datadog #280

Merged
merged 9 commits into from
Jun 28, 2024
Merged

Conversation

muhammadadeeltajamul
Copy link
Contributor

Removed new relic reporting and added datadog reporting to edx-ace
Added edx-django-utils and ddtrace

Test this by creating edx-platform PR with this package version

except ImportError:
newrelic = None # pylint: disable=invalid-name
ddtrace = None # pylint: disable=invalid-name
from edx_django_utils.monitoring import DatadogBackend
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a good opportunity to stop referring to specific telemetry directly, and to start using edx-django-utils's abstraction layer as described at https://github.com/openedx/edx-django-utils/tree/master/edx_django_utils/monitoring (there's no need to set OPENEDX_TELEMETRY, since the IDA sets that, not the library). Here's what that would look like:

  1. Delete the try block above
  2. On this line, instead do from edx_django_utils.monitoring import set_custom_attribute
  3. Delete report_to_datadog
  4. Change report to call set_custom_attribute
  5. Delete ddtrace from the requirements

@@ -7,3 +7,5 @@ attrs>=17.2.0 # Attributes without boilerplate
sailthru-client==2.2.3
six
stevedore>=1.10.0
ddtrace==2.8.5
edx-django-utils==5.14.2
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're pinning edx-django-utils to a specific version, it should be explained in a comment and link to a ticket for removing the pin when the situation allows. Min-version constraints are fine, but capping the version interferes with routine upgrades and can cause "dependency hell" later. You can read more about this in OEP-18:

https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0018-bp-python-dependencies.html#imposing-constraints-on-dependencies

Copy link
Contributor

@timmc-edx timmc-edx left a comment

Choose a reason for hiding this comment

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

Looks good, although remember to update the changelog if you're planning on doing that post-release.

@muhammadadeeltajamul muhammadadeeltajamul merged commit 9644543 into master Jun 28, 2024
9 checks passed
@muhammadadeeltajamul muhammadadeeltajamul deleted the inf-1410 branch June 28, 2024 06:28
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.

3 participants