-
Notifications
You must be signed in to change notification settings - Fork 127
Django middleware filtering settings #1444
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
Conversation
* Do not wrap useless middlewares * Fixup: use frozenset
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1444 +/- ##
==========================================
+ Coverage 81.33% 81.42% +0.09%
==========================================
Files 209 209
Lines 23650 23709 +59
Branches 3724 3740 +16
==========================================
+ Hits 19236 19306 +70
+ Misses 3153 3146 -7
+ Partials 1261 1257 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posting a partial review-I need to look at the tests and the include/exclude implementation a little bit deeper.
newrelic/core/config.py
Outdated
@@ -644,6 +649,17 @@ def _parse_attributes(s): | |||
return valid | |||
|
|||
|
|||
# Called from newrelic.config.py to parse django_middleware lists | |||
def _parse_django_middleware(s): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just point at this method rather than re-creating the exact same implementation again: _parse_attributes
? Maybe just rename it to be generic or accept an additional argument for the log message.
newrelic/hooks/framework_django.py
Outdated
if exclude_middleware.endswith("*"): | ||
name = exclude_middleware.rstrip("*") | ||
if callable_name.startswith(name): | ||
if not include_logic(callable_name, name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cache the value so we aren't computing it twice.
if not include_logic(callable_name, name): | |
include = include_logic(callable_name, name) | |
if not include: | |
return False | |
else: | |
deny |= include |
newrelic/hooks/framework_django.py
Outdated
""" | ||
if len(settings.instrumentation.django_middleware.include) == 0: | ||
return True | ||
for include_middleware in settings.instrumentation.django_middleware.include: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My brain is fried from trying to understand this...I'll come back to this later and hopefully it will make more sense. In theory I see what you are trying to do here but my brain is not following all of these implementation details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair--it gets a bit dicey. A lot of the complexity in this logic comes from attempting to sort out a case like this one:
Middleware = django.middleware.common:CommonMiddleware, django.middleware.csrf:CsrfViewMiddleware
exclude = ["django.middleware.c*"],
include = ["django.middleware.", "django.middleware.co"]
The django.middleware.*
in the include list is effectively a no op. However, in the case where a user has this scenario, this should still filter correctly (based on the more specific include statement, django.middleware.co*
, relative to the exclude statement django.middleware.c*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I mainly just want some more thorough tests for the filtering-otherwise this looks good.
"instrumentation.django_middleware.enabled": True, | ||
} | ||
|
||
wildcard_exclude_specific_include_settings = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get some more tests here:
- there aren't *'d paths.
- there is only an exclude rule
- there is only an include rule
- there are no dots in the paths
- there are only dots in the include path
- there are only dots in the exclude path
- there is a path with a dot (no *) and a path with no dots and only a *
- case sensitive test
There's probably some additional cases I'm not thinking of?
I want to be really thorough when we test these cause there's a lot of complex logic there and I don't want to miss some corner case bug in the implementation. (I don't see any but it might be hard to catch without thorough testing.)
* Bump the github_actions group with 2 updates (#1466) Bumps the github_actions group with 2 updates: [codecov/codecov-action](https://github.com/codecov/codecov-action) and [github/codeql-action](https://github.com/github/codeql-action). Updates `codecov/codecov-action` from 5.4.3 to 5.5.0 - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@18283e0...fdcc847) Updates `github/codeql-action` from 3.29.10 to 3.29.11 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@96f518a...3c3833e) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-version: 5.5.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: github_actions - dependency-name: github/codeql-action dependency-version: 3.29.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github_actions ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * Safeguards for deepest unique path (#1450) * Safeguards for deepest unique path * Remove breakpoint --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * Add try-except to web request parsing (#1449) * Add try-except to web request parsing * Fix parsing logic * Add clarifying comment --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * Use legacy bitnami for now (#1471) * Use legacy bitnami for now * Revert solr change * Revert zookeeper change * Add graphene-django instrumentation (#1451) * Add graphene-django instrumentation * Increase naming priority * Remove unused import * Add sychronous schema tests * Clean up test files * Remove commented out code * Megalinter fixes * Add operation & resolver tests * Refine tests * MegaLinter fixes * Suggested reviewer changes * Megalinter fixes * Django middleware filtering settings (#1444) * Reduce number of spans in django framework (#779) * Do not wrap useless middlewares * Fixup: use frozenset * Add config settings * Add middleware enable/disable options * Add testing * Testing exclude/include settings * Add optional fixture scope argument * Rewrite tests to use fixtures * Add new fixture * Fix tests * Fix ruff errors * MegaLinter Fixes * Add config file tests * MegaLinter fixes * Reviewer changes * MegaLinter fixes * Add InstrumentationMiddlewareSettings * More exclude/include filter tests * Megalinter fixes * Tests to increase coverage * Megalinter fixes * More coverage tests * ANOTHER TEST: --------- Co-authored-by: Hannah Stepanek <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * Pin bitnami images to bitnamilegacy (#1475) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * Distributed CI Image Build (#1478) * Distribute build of CI image across runners * Add weekly CI image rebuild * Add rust to toolchain * Add human readable job names * Linting --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Lalleh Rafeei <[email protected]> Co-authored-by: Hannah Stepanek <[email protected]>
This PR adds three settings related to Django middleware instrumentation:
instrumentation.middleware.django.enabled
instrumentation.middleware.django.exclude
instrumentation.middleware.django.include
Where the exclude/include settings follow similar rules to attribute exclude/include: https://docs.newrelic.com/docs/apm/agents/python-agent/attributes/enabling-disabling-attributes-python/#rule-specific-wins
instrumentation.middleware.django.enabled
is disabled, no middlewares are recorded, despite what may be in theinclude
list*
denotes a wildcard. NOTE: This will not work as a pure wildcard, as it will only work at the end of a name and not in the middle or beginning.i.e.
Middleware seen in application: "foo", "bar", "baz"
instrumentation.middleware.django.enabled
= Trueinstrumentation.middleware.django.exclude
= ["ba*"]instrumentation.middleware.django.include
= ["baz"]Middleware that is included in instrumentation: "foo", "baz"
Middleware that is excluded in instrumentation: "bar"