-
Notifications
You must be signed in to change notification settings - Fork 126
Remove capture_params setting #1485
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
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-11.0.0 #1485 +/- ##
=================================================
Coverage ? 65.05%
=================================================
Files ? 204
Lines ? 23546
Branches ? 3717
=================================================
Hits ? 15318
Misses ? 6904
Partials ? 1324 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c5942b6
to
c47d692
Compare
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.
Haven't finished reading the whole thing yet, but I think this might need some redesign so posting these initial comments early.
newrelic/api/web_transaction.py
Outdated
# While settings.capture_params has been removed, the Python Agent | ||
# has not removed the newrelic specific environ setting for | ||
# capturing request parameters. | ||
self._capture_request_params = _lookup_environ_setting(environ, "newrelic.capture_request_params", None) | ||
|
||
if self._capture_request_params: | ||
self.settings.attributes.include.append("request.parameters.*") | ||
|
||
# Make sure that if high security mode is enabled that | ||
# capture of request params is still being disabled. | ||
# No warning is issued for this in the logs because it | ||
# is a per request configuration and would create a lot | ||
# of noise. | ||
|
||
if settings.high_security: | ||
self.capture_params = False | ||
self.settings.attributes.exclude.append("request.parameters.*") |
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.
This section's behavior seems potentially incorrect with regards to high security. If high security mode is enabled, there shouldn't be any params captured. If it's not, THEN we could add the request params to the include list. Otherwise, we're going to see some conflicting settings for include and exclude where it's unclear which one should win.
Beyond that, this isn't correct as a user can set the include list to have a specific param included, and it will be included despite the exclude list set to *
. This is because the attribute system prioritizes more specific over general when rules conflict.
This code seems like it's just recreating the behavior of capture_params
under the hood, when really we should be taking it out altogether. Can we just remove this _capture_request_params
attribute and replace it with:
- Checking for high security mode, and not allowing any param captures at all if enabled.
- Capturing the params and putting them through the attribute filter.
I'm not sure what the defaults are here and we may need to consider them, but that seems like the most sensible behavior.
newrelic/api/web_transaction.py
Outdated
# Don't add request parameters at all, which means | ||
# they will not go through the AttributeFilter. | ||
if "request.parameters.*" in self.settings.attributes.exclude: | ||
self._request_params.clear() |
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.
This is not correct. The above explanation applies, where a user can set all params to be excluded with exclude set to request.parameters.*
, but then include only specific params with include settings like request.parameters.my_param
.
The current change breaks this functionality by not capturing them in the first place, so the more specific rules won't win like they're supposed to.
Edited the PR description for more information, but the work on this will be deferred to the next deprecation cycle. |
EDIT: The work on this PR will be deferred to the next deprecation removal cycle.
The future intent is to deprecate the following:
capture_params
settingnewrelic.capture_request_params
and any other "newrelic.*" WSGI environ dictionary keys since there are APIs that perform the same taskscapture_request_params()
APISome background on the agent attributes and the request_parameters that get added:
transaction._request_params
dict is populated inWebTransaction.__init__
if HSM is disabledtransaction.request_parameters
is a property that returns thetransaction._request_params
dict with keys that are prefaced with “request.parameters.”transaction.agent_attributes
is a property that takestransaction._agent_attributes
dict and runs it throughsettings.attribute_filter
in Transaction.exit:
self._update_agent_attributes() # adds more to self._agent_attributes dict
root_agent_attributes = dict(self._agent_attributes)
root_agent_attributes.update(self.request_parameters)
...
agent_attributes = self.agent_attributes # takes self._agent_attributes and runs it through attribute_filter
agent_attributes.extend(self.filter_request_parameters(self.request_parameters))
root_agent_attributes
is populated with unfilteredself._agent_attributes
andself.request_parameters
agent_attributes
is populated with filtered versions ofself._agent_attributes
andself.request_parameters
as seen byself.agent_attributes
andself.filter_request_parameters(self.request_parameters)
, respectively.If the intention is to keep
capture_request_params()
or have some version of this, the application attribute settings should not be modified. Instead, circumvent this by updatingself._agent_attributes
to includeself.request_parameters()
whencapture_request_params()
isTrue
, and clearself._request_params
whencapture_request_params()
is set toFalse
And where HSM is enabled, put some version of this code for all
DESTINATION.attributes.include
inapply_local_high_security_mode_setting()
to strip any variation of the "request.parameters.*" filters:===========
This PR removes the deprecated
capture_params
setting.While this setting has been removed, there are two settings that heavily relied upon this setting that were not removed:
newrelic.capture_request_params
capture_request_params
APIRequest parameters are a special kind of agent attribute that is prefaced with
request.parameters.
—with settings such as HSM, these need to be disabled. The analogous settings forcapture_params
are as follows:capture_params = True:
attributes.enabled = True
attributes.include=["request.parameters.*"]
capture_params = False:
attributes.enabled=True
attributes.exclude=["request.parameters.*"]
(Default behavior, where the collection of request.parameters is disabled by default)
capture_params = None:
attributes.enabled=True