From a57f789bb8dd2cec05a3806aa69a13ba31da5939 Mon Sep 17 00:00:00 2001 From: Hannah Stepanek Date: Thu, 15 Feb 2024 12:31:23 -0800 Subject: [PATCH 1/9] Drop None type attributes --- newrelic/core/attribute.py | 33 +++++++++++++++++++ tests/agent_features/test_custom_events.py | 19 +++++++++++ .../test_dimensional_metrics.py | 1 + tests/agent_features/test_error_events.py | 5 ++- tests/agent_features/test_log_events.py | 3 +- tests/agent_features/test_time_trace.py | 30 +++++++++++++++++ ...n_event_data_and_some_browser_stuff_too.py | 5 ++- 7 files changed, 93 insertions(+), 3 deletions(-) diff --git a/newrelic/core/attribute.py b/newrelic/core/attribute.py index 04dfdec34..bbccafda5 100644 --- a/newrelic/core/attribute.py +++ b/newrelic/core/attribute.py @@ -110,6 +110,10 @@ class CastingFailureException(Exception): pass +class NullValueException(ValueError): + pass + + class Attribute(_Attribute): def __repr__(self): return "Attribute(name=%r, value=%r, destinations=%r)" % (self.name, self.value, bin(self.destinations)) @@ -285,6 +289,15 @@ def process_user_attribute(name, value, max_length=MAX_ATTRIBUTE_LENGTH, ending= _logger.debug("Attribute value cannot be cast to a string. Dropping attribute: %r=%r", name, value) return FAILED_RESULT + except NullValueException: + _logger.debug( + "Attribute value is None. There is no difference between omitting the key " + "and sending None. Dropping attribute: %r=%r", + name, + value, + ) + return FAILED_RESULT + else: # Check length after casting @@ -311,9 +324,29 @@ def sanitize(value): Insights. Otherwise, convert value to a string. Raise CastingFailureException, if str(value) somehow fails. + Raise NullValueException, if value is None (null values SHOULD NOT be reported). """ valid_value_types = (six.text_type, six.binary_type, bool, float, six.integer_types) + # The agent spec says: + # Agents **SHOULD NOT** report `null` attribute values. The behavior of `IS NULL` + # queries in insights makes it so that omitted keys behave the same as `null` + # keys. Since there is no difference between omitting the key and sending a + # `null, we **SHOULD** reduce the payload size by omitting `null` values from the + # payload entirely. + # + # Since there should not be a difference in behavior for customers recording + # `null` attributes versus customers omitting an attribute, agents **SHOULD** add + # debug logging when a null value is recorded. This will give agents observability + # on recording of `null` attributes outside of audit logs. + # + # "empty" values such as `""` and `0` **MUST** be sent as an attribute in the + # payload. + if value is None: + raise NullValueException( + "Attribute value is of type: None. Omitting value since there is " + "no difference between omitting the key and sending None." + ) # When working with numpy, note that numpy has its own `int`s, `str`s, # et cetera. `numpy.str_` and `numpy.float_` inherit from Python's native diff --git a/tests/agent_features/test_custom_events.py b/tests/agent_features/test_custom_events.py index d03feea29..afc58da51 100644 --- a/tests/agent_features/test_custom_events.py +++ b/tests/agent_features/test_custom_events.py @@ -59,6 +59,25 @@ def test_process_event_type_name_invalid_chars(): _user_params = {'foo': 'bar'} _event = [_intrinsics, _user_params] +@reset_core_stats_engine() +@validate_custom_event_in_application_stats_engine(_event) +@background_task() +def test_add_custom_event_to_transaction_stats_engine_drops_none_attr(): + attrs = {"drop-me": None} + attrs.update(_user_params) + record_custom_event("FooEvent", attrs) + + +@reset_core_stats_engine() +@validate_custom_event_in_application_stats_engine(_event) +def test_add_custom_event_to_application_stats_engine_drops_none_attr(): + attrs = {"drop-me": None} + attrs.update(_user_params) + + app = application() + record_custom_event("FooEvent", attrs, application=app) + + @reset_core_stats_engine() @validate_custom_event_in_application_stats_engine(_event) @background_task() diff --git a/tests/agent_features/test_dimensional_metrics.py b/tests/agent_features/test_dimensional_metrics.py index ef9e98418..f3510e7dd 100644 --- a/tests/agent_features/test_dimensional_metrics.py +++ b/tests/agent_features/test_dimensional_metrics.py @@ -62,6 +62,7 @@ def otlp_content_encoding(request): _test_tags_examples = [ (None, None), ({}, None), + ({"drop-me": None}, None), ([], None), ({"str": "a"}, frozenset({("str", "a")})), ({"int": 1}, frozenset({("int", 1)})), diff --git a/tests/agent_features/test_error_events.py b/tests/agent_features/test_error_events.py index e7d40145f..b9ab99b58 100644 --- a/tests/agent_features/test_error_events.py +++ b/tests/agent_features/test_error_events.py @@ -247,11 +247,14 @@ def test_error_event_outside_transaction(): @reset_core_stats_engine() @validate_non_transaction_error_event(_intrinsic_attributes, required_user=_err_params) def test_error_event_with_params_outside_transaction(): + attrs = {"drop-me": None} + attrs.update(_err_params) + try: raise outside_error except ErrorEventOutsideTransactionError: app = application() - notice_error(sys.exc_info(), attributes=_err_params, application=app) + notice_error(sys.exc_info(), attributes=attrs, application=app) @reset_core_stats_engine() diff --git a/tests/agent_features/test_log_events.py b/tests/agent_features/test_log_events.py index fb9991a82..2fc709233 100644 --- a/tests/agent_features/test_log_events.py +++ b/tests/agent_features/test_log_events.py @@ -69,7 +69,8 @@ def exercise_record_log_event(): set_trace_ids() record_log_event("no_other_arguments") - record_log_event("keyword_arguments", timestamp=1234, level="ERROR", attributes={"key": "value"}) + # Attributes with value None should be dropped. + record_log_event("keyword_arguments", timestamp=1234, level="ERROR", attributes={"key": "value", "drop-me": None}) record_log_event("positional_arguments", "WARNING", 2345, {"key": "value"}) record_log_event("serialized_attributes", attributes=_serialized_attributes) record_log_event(None, attributes={"attributes_only": "value"}) diff --git a/tests/agent_features/test_time_trace.py b/tests/agent_features/test_time_trace.py index eccb4d7fe..d86634386 100644 --- a/tests/agent_features/test_time_trace.py +++ b/tests/agent_features/test_time_trace.py @@ -15,6 +15,8 @@ import logging import pytest +from testing_support.fixtures import dt_enabled +from testing_support.validators.validate_span_events import validate_span_events from testing_support.validators.validate_transaction_metrics import ( validate_transaction_metrics, ) @@ -71,3 +73,31 @@ def test_trace_finalizes_with_transaction_missing_settings(monkeypatch, trace_ty # Ensure transaction still has settings when it exits to prevent other crashes making errors hard to read monkeypatch.undo() assert txn.settings + + +@pytest.mark.parametrize( + "trace_type,args", + ( + (DatabaseTrace, ("select * from foo",)), + (DatastoreTrace, ("db_product", "db_target", "db_operation")), + (ExternalTrace, ("lib", "url")), + (FunctionTrace, ("name",)), + (GraphQLOperationTrace, ()), + (GraphQLResolverTrace, ()), + (MemcacheTrace, ("command",)), + (MessageTrace, ("lib", "operation", "dst_type", "dst_name")), + (SolrTrace, ("lib", "command")), + ), +) +@dt_enabled +@validate_span_events( + count=1, + expected_users=["foo"], + unexpected_users=["drop-me"], +) +@background_task() +def test_trace_filters_out_invalid_attributes(trace_type, args): + txn = current_transaction() + with trace_type(*args) as trace: + trace.add_custom_attribute("drop-me", None) + trace.add_custom_attribute("foo", "bar") diff --git a/tests/agent_features/test_transaction_event_data_and_some_browser_stuff_too.py b/tests/agent_features/test_transaction_event_data_and_some_browser_stuff_too.py index c1d9283c2..55711d694 100644 --- a/tests/agent_features/test_transaction_event_data_and_some_browser_stuff_too.py +++ b/tests/agent_features/test_transaction_event_data_and_some_browser_stuff_too.py @@ -29,6 +29,7 @@ from newrelic.api.application import application_settings from newrelic.api.background_task import background_task +from newrelic.api.transaction import current_transaction from newrelic.common.encoding_utils import deobfuscate from newrelic.common.object_wrapper import transient_function_wrapper @@ -491,7 +492,7 @@ def test_database_and_external_attributes_in_analytics(): } _expected_absent_attributes = { - "user": ("foo"), + "user": ("foo", "drop-me"), "agent": ("response.status", "request.method"), "intrinsic": ("port"), } @@ -500,4 +501,6 @@ def test_database_and_external_attributes_in_analytics(): @validate_transaction_event_attributes(_expected_attributes, _expected_absent_attributes) @background_task() def test_background_task_intrinsics_has_no_port(): + transaction = current_transaction() + transaction.add_custom_attribute("drop-me", None) pass From 22acf04114c6d335ae2d25063e87d2829d78088f Mon Sep 17 00:00:00 2001 From: Hannah Stepanek Date: Thu, 15 Feb 2024 17:39:16 -0800 Subject: [PATCH 2/9] Remove unused imports --- newrelic/core/root_node.py | 41 +++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/newrelic/core/root_node.py b/newrelic/core/root_node.py index 63acf6174..2cc84cf33 100644 --- a/newrelic/core/root_node.py +++ b/newrelic/core/root_node.py @@ -16,30 +16,39 @@ import newrelic.core.trace_node from newrelic.core.node_mixin import GenericNodeMixin -from newrelic.core.attribute import resolve_user_attributes -from newrelic.packages import six - -_RootNode = namedtuple('_RootNode', - ['name', 'children', 'start_time', 'end_time', 'exclusive', - 'duration', 'guid', 'agent_attributes', 'user_attributes', - 'path', 'trusted_parent_span', 'tracing_vendors',]) +_RootNode = namedtuple( + "_RootNode", + [ + "name", + "children", + "start_time", + "end_time", + "exclusive", + "duration", + "guid", + "agent_attributes", + "user_attributes", + "path", + "trusted_parent_span", + "tracing_vendors", + ], +) class RootNode(_RootNode, GenericNodeMixin): def span_event(self, *args, **kwargs): span = super(RootNode, self).span_event(*args, **kwargs) i_attrs = span[0] - i_attrs['transaction.name'] = self.path - i_attrs['nr.entryPoint'] = True + i_attrs["transaction.name"] = self.path + i_attrs["nr.entryPoint"] = True if self.trusted_parent_span: - i_attrs['trustedParentId'] = self.trusted_parent_span + i_attrs["trustedParentId"] = self.trusted_parent_span if self.tracing_vendors: - i_attrs['tracingVendors'] = self.tracing_vendors + i_attrs["tracingVendors"] = self.tracing_vendors return span def trace_node(self, stats, root, connections): - name = self.path start_time = newrelic.core.trace_node.node_start_time(root, self) @@ -57,9 +66,5 @@ def trace_node(self, stats, root, connections): params = self.get_trace_segment_params(root.settings) return newrelic.core.trace_node.TraceNode( - start_time=start_time, - end_time=end_time, - name=name, - params=params, - children=children, - label=None) + start_time=start_time, end_time=end_time, name=name, params=params, children=children, label=None + ) From 62b1bc0ee8a4acd01e3fe9d5c923a15f0f2049b9 Mon Sep 17 00:00:00 2001 From: Hannah Stepanek Date: Thu, 15 Feb 2024 17:41:40 -0800 Subject: [PATCH 3/9] Only add attr if key is not None Now we can remove the check for None inside resolve_user_attributes since process_user_attribute is always called on the attributes before they are passed to resolve_user_attributes. This None check is unreachable code and not needed. --- newrelic/core/attribute.py | 3 - newrelic/core/node_mixin.py | 111 ++++++++++++++---------------------- 2 files changed, 44 insertions(+), 70 deletions(-) diff --git a/newrelic/core/attribute.py b/newrelic/core/attribute.py index bbccafda5..45df72268 100644 --- a/newrelic/core/attribute.py +++ b/newrelic/core/attribute.py @@ -150,9 +150,6 @@ def resolve_user_attributes(attr_dict, attribute_filter, target_destination, att u_attrs = attr_class() for attr_name, attr_value in attr_dict.items(): - if attr_value is None: - continue - dest = attribute_filter.apply(attr_name, DST_ALL) if dest & target_destination: diff --git a/newrelic/core/node_mixin.py b/newrelic/core/node_mixin.py index b7c5ed69c..944494439 100644 --- a/newrelic/core/node_mixin.py +++ b/newrelic/core/node_mixin.py @@ -13,141 +13,118 @@ # limitations under the License. import newrelic.core.attribute as attribute - -from newrelic.core.attribute_filter import (DST_SPAN_EVENTS, - DST_TRANSACTION_SEGMENTS) +from newrelic.core.attribute_filter import DST_SPAN_EVENTS, DST_TRANSACTION_SEGMENTS class GenericNodeMixin(object): @property def processed_user_attributes(self): - if hasattr(self, '_processed_user_attributes'): + if hasattr(self, "_processed_user_attributes"): return self._processed_user_attributes self._processed_user_attributes = u_attrs = {} - user_attributes = getattr(self, 'user_attributes', u_attrs) + user_attributes = getattr(self, "user_attributes", u_attrs) for k, v in user_attributes.items(): k, v = attribute.process_user_attribute(k, v) - u_attrs[k] = v + # Only record the attribute if it passes processing. + # Failures return (None, None). + if k: + u_attrs[k] = v return u_attrs def get_trace_segment_params(self, settings, params=None): _params = attribute.resolve_agent_attributes( - self.agent_attributes, - settings.attribute_filter, - DST_TRANSACTION_SEGMENTS) + self.agent_attributes, settings.attribute_filter, DST_TRANSACTION_SEGMENTS + ) if params: _params.update(params) - _params.update(attribute.resolve_user_attributes( - self.processed_user_attributes, - settings.attribute_filter, - DST_TRANSACTION_SEGMENTS)) + _params.update( + attribute.resolve_user_attributes( + self.processed_user_attributes, settings.attribute_filter, DST_TRANSACTION_SEGMENTS + ) + ) - _params['exclusive_duration_millis'] = 1000.0 * self.exclusive + _params["exclusive_duration_millis"] = 1000.0 * self.exclusive return _params - def span_event( - self, - settings, - base_attrs=None, - parent_guid=None, - attr_class=dict): + def span_event(self, settings, base_attrs=None, parent_guid=None, attr_class=dict): i_attrs = base_attrs and base_attrs.copy() or attr_class() - i_attrs['type'] = 'Span' - i_attrs['name'] = self.name - i_attrs['guid'] = self.guid - i_attrs['timestamp'] = int(self.start_time * 1000) - i_attrs['duration'] = self.duration - i_attrs['category'] = 'generic' + i_attrs["type"] = "Span" + i_attrs["name"] = self.name + i_attrs["guid"] = self.guid + i_attrs["timestamp"] = int(self.start_time * 1000) + i_attrs["duration"] = self.duration + i_attrs["category"] = "generic" if parent_guid: - i_attrs['parentId'] = parent_guid + i_attrs["parentId"] = parent_guid a_attrs = attribute.resolve_agent_attributes( - self.agent_attributes, - settings.attribute_filter, - DST_SPAN_EVENTS, - attr_class=attr_class) + self.agent_attributes, settings.attribute_filter, DST_SPAN_EVENTS, attr_class=attr_class + ) u_attrs = attribute.resolve_user_attributes( - self.processed_user_attributes, - settings.attribute_filter, - DST_SPAN_EVENTS, - attr_class=attr_class) + self.processed_user_attributes, settings.attribute_filter, DST_SPAN_EVENTS, attr_class=attr_class + ) # intrinsics, user attrs, agent attrs return [i_attrs, u_attrs, a_attrs] - def span_events(self, - settings, base_attrs=None, parent_guid=None, attr_class=dict): - - yield self.span_event( - settings, - base_attrs=base_attrs, - parent_guid=parent_guid, - attr_class=attr_class) + def span_events(self, settings, base_attrs=None, parent_guid=None, attr_class=dict): + yield self.span_event(settings, base_attrs=base_attrs, parent_guid=parent_guid, attr_class=attr_class) for child in self.children: for event in child.span_events( - settings, - base_attrs=base_attrs, - parent_guid=self.guid, - attr_class=attr_class): + settings, base_attrs=base_attrs, parent_guid=self.guid, attr_class=attr_class + ): yield event class DatastoreNodeMixin(GenericNodeMixin): - @property def name(self): product = self.product target = self.target - operation = self.operation or 'other' + operation = self.operation or "other" if target: - name = 'Datastore/statement/%s/%s/%s' % (product, target, - operation) + name = "Datastore/statement/%s/%s/%s" % (product, target, operation) else: - name = 'Datastore/operation/%s/%s' % (product, operation) + name = "Datastore/operation/%s/%s" % (product, operation) return name @property def db_instance(self): - if hasattr(self, '_db_instance'): + if hasattr(self, "_db_instance"): return self._db_instance db_instance_attr = None if self.database_name: - _, db_instance_attr = attribute.process_user_attribute( - 'db.instance', self.database_name) + _, db_instance_attr = attribute.process_user_attribute("db.instance", self.database_name) self._db_instance = db_instance_attr return db_instance_attr def span_event(self, *args, **kwargs): - self.agent_attributes['db.instance'] = self.db_instance + self.agent_attributes["db.instance"] = self.db_instance attrs = super(DatastoreNodeMixin, self).span_event(*args, **kwargs) i_attrs = attrs[0] a_attrs = attrs[2] - i_attrs['category'] = 'datastore' - i_attrs['component'] = self.product - i_attrs['span.kind'] = 'client' + i_attrs["category"] = "datastore" + i_attrs["component"] = self.product + i_attrs["span.kind"] = "client" if self.instance_hostname: - _, a_attrs['peer.hostname'] = attribute.process_user_attribute( - 'peer.hostname', self.instance_hostname) + _, a_attrs["peer.hostname"] = attribute.process_user_attribute("peer.hostname", self.instance_hostname) else: - a_attrs['peer.hostname'] = 'Unknown' + a_attrs["peer.hostname"] = "Unknown" - peer_address = '%s:%s' % ( - self.instance_hostname or 'Unknown', - self.port_path_or_id or 'Unknown') + peer_address = "%s:%s" % (self.instance_hostname or "Unknown", self.port_path_or_id or "Unknown") - _, a_attrs['peer.address'] = attribute.process_user_attribute( - 'peer.address', peer_address) + _, a_attrs["peer.address"] = attribute.process_user_attribute("peer.address", peer_address) return attrs From 5bb045bf5e391441aee03937427bbd8817b7f2dd Mon Sep 17 00:00:00 2001 From: Hannah Stepanek Date: Thu, 15 Feb 2024 19:02:05 -0800 Subject: [PATCH 4/9] Replace create_user_attributes w/create_attributes create_user_attributes is a passthrough to create_attributes. Just remove this function, there's no need for it. --- newrelic/api/transaction.py | 4 ++-- newrelic/core/attribute.py | 11 ++++++----- newrelic/core/stats_engine.py | 6 +++--- newrelic/core/transaction_node.py | 12 ++++-------- 4 files changed, 15 insertions(+), 18 deletions(-) diff --git a/newrelic/api/transaction.py b/newrelic/api/transaction.py index 5e8ee2e72..0b574feb0 100644 --- a/newrelic/api/transaction.py +++ b/newrelic/api/transaction.py @@ -52,12 +52,12 @@ MAX_NUM_USER_ATTRIBUTES, create_agent_attributes, create_attributes, - create_user_attributes, process_user_attribute, resolve_logging_context_attributes, truncate, ) from newrelic.core.attribute_filter import ( + DST_ALL, DST_ERROR_COLLECTOR, DST_NONE, DST_TRANSACTION_TRACER, @@ -1004,7 +1004,7 @@ def _update_agent_attributes(self): @property def user_attributes(self): - return create_user_attributes(self._custom_params, self.attribute_filter) + return create_attributes(self._custom_params, DST_ALL, self.attribute_filter) def _compute_sampled_and_priority(self): if self._priority is None: diff --git a/newrelic/core/attribute.py b/newrelic/core/attribute.py index 45df72268..c80a5cbc2 100644 --- a/newrelic/core/attribute.py +++ b/newrelic/core/attribute.py @@ -147,6 +147,12 @@ def create_agent_attributes(attr_dict, attribute_filter): def resolve_user_attributes(attr_dict, attribute_filter, target_destination, attr_class=dict): + """ + Returns an attr_class of key value attributes filtered to the target_destination. + + process_user_attribute MUST be called before this function to filter out invalid + attributes. + """ u_attrs = attr_class() for attr_name, attr_value in attr_dict.items(): @@ -202,11 +208,6 @@ def resolve_logging_context_attributes(attr_dict, attribute_filter, attr_prefix, return c_attrs -def create_user_attributes(attr_dict, attribute_filter): - destinations = DST_ALL - return create_attributes(attr_dict, destinations, attribute_filter) - - def truncate(text, maxsize=MAX_ATTRIBUTE_LENGTH, encoding="utf-8", ending=None): # Truncate text so that its byte representation # is no longer than maxsize bytes. diff --git a/newrelic/core/stats_engine.py b/newrelic/core/stats_engine.py index 6e6e6155a..aef6b4408 100644 --- a/newrelic/core/stats_engine.py +++ b/newrelic/core/stats_engine.py @@ -41,12 +41,12 @@ from newrelic.core.attribute import ( MAX_LOG_MESSAGE_LENGTH, create_agent_attributes, - create_user_attributes, + create_attributes, process_user_attribute, resolve_logging_context_attributes, truncate, ) -from newrelic.core.attribute_filter import DST_ERROR_COLLECTOR +from newrelic.core.attribute_filter import DST_ALL, DST_ERROR_COLLECTOR from newrelic.core.code_level_metrics import extract_code_from_traceback from newrelic.core.config import is_expected_error, should_ignore_error from newrelic.core.database_utils import explain_plan @@ -824,7 +824,7 @@ def notice_error(self, error=None, attributes=None, expected=None, ignore=None, ) custom_attributes = {} - user_attributes = create_user_attributes(custom_attributes, settings.attribute_filter) + user_attributes = create_attributes(custom_attributes, DST_ALL, settings.attribute_filter) # Extract additional details about the exception as agent attributes agent_attributes = {} diff --git a/newrelic/core/transaction_node.py b/newrelic/core/transaction_node.py index 5aa9fd84a..bafbb7a96 100644 --- a/newrelic/core/transaction_node.py +++ b/newrelic/core/transaction_node.py @@ -24,8 +24,9 @@ import newrelic.core.trace_node from newrelic.common.encoding_utils import camel_case from newrelic.common.streaming_utils import SpanProtoAttrs -from newrelic.core.attribute import create_agent_attributes, create_user_attributes +from newrelic.core.attribute import create_agent_attributes, create_attributes from newrelic.core.attribute_filter import ( + DST_ALL, DST_ERROR_COLLECTOR, DST_TRANSACTION_EVENTS, DST_TRANSACTION_TRACER, @@ -367,7 +368,7 @@ def error_details(self): if attr.destinations & DST_ERROR_COLLECTOR: params["agentAttributes"][attr.name] = attr.value - err_attrs = create_user_attributes(error.custom_params, self.settings.attribute_filter) + err_attrs = create_attributes(error.custom_params, DST_ALL, self.settings.attribute_filter) for attr in err_attrs: if attr.destinations & DST_ERROR_COLLECTOR: params["userAttributes"][attr.name] = attr.value @@ -381,7 +382,6 @@ def error_details(self): ) def transaction_trace(self, stats, limit, connections): - self.trace_node_count = 0 self.trace_node_limit = limit @@ -508,10 +508,8 @@ def _add_if_not_empty(key, value): return intrinsics def error_events(self, stats_table): - errors = [] for error in self.errors: - intrinsics = self.error_event_intrinsics(error, stats_table) # Add user and agent attributes to event @@ -540,7 +538,7 @@ def error_events(self, stats_table): # add error specific custom params to this error's userAttributes - err_attrs = create_user_attributes(error.custom_params, self.settings.attribute_filter) + err_attrs = create_attributes(error.custom_params, DST_ALL, self.settings.attribute_filter) for attr in err_attrs: if attr.destinations & DST_ERROR_COLLECTOR: user_attributes[attr.name] = attr.value @@ -551,7 +549,6 @@ def error_events(self, stats_table): return errors def error_event_intrinsics(self, error, stats_table): - intrinsics = self._event_intrinsics(stats_table) intrinsics["type"] = "TransactionError" @@ -573,7 +570,6 @@ def _event_intrinsics(self, stats_table): cache = getattr(self, "_event_intrinsics_cache", None) if cache is not None: - # We don't want to execute this function more than once, since # it should always yield the same data per transaction From 0337ab6262f99f746159a2f964346b8059664f9d Mon Sep 17 00:00:00 2001 From: Hannah Stepanek Date: Thu, 15 Feb 2024 19:15:19 -0800 Subject: [PATCH 5/9] Add description of create_agent_attributes --- newrelic/core/attribute.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/newrelic/core/attribute.py b/newrelic/core/attribute.py index c80a5cbc2..77cc83feb 100644 --- a/newrelic/core/attribute.py +++ b/newrelic/core/attribute.py @@ -130,6 +130,13 @@ def create_attributes(attr_dict, destinations, attribute_filter): def create_agent_attributes(attr_dict, attribute_filter): + """ + Returns a dictionary of Attribute objects with appropriate destinations. + + If the attribute's key is in the known list of event attributes, it is assigned + to _DESTINATIONS_WITH_EVENTS, otherwise it is assigned to _DESTINATIONS. + Note attributes with a value of None are filtered out. + """ attributes = [] for k, v in attr_dict.items(): From ee87f7affb9e421977943f5a337afdadc3bfe0e7 Mon Sep 17 00:00:00 2001 From: Hannah Stepanek Date: Thu, 15 Feb 2024 20:47:16 -0800 Subject: [PATCH 6/9] Remove log context attr=None tests --- tests/logger_logging/test_attributes.py | 2 +- tests/logger_loguru/test_attributes.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/logger_logging/test_attributes.py b/tests/logger_logging/test_attributes.py index 2a8240e12..63a7e94f7 100644 --- a/tests/logger_logging/test_attributes.py +++ b/tests/logger_logging/test_attributes.py @@ -58,7 +58,7 @@ def test_logging_extra_attributes(logger): logger.error("extras", extra={"extra_attr": 1}) -@validate_log_events([{"message": "exc_info"}], required_attrs=["context.exc_info", "context.exc_text"]) +@validate_log_events([{"message": "exc_info"}], required_attrs=["context.exc_info"]) @validate_log_event_count(1) @background_task() def test_logging_exc_info_context_attributes(logger): diff --git a/tests/logger_loguru/test_attributes.py b/tests/logger_loguru/test_attributes.py index ff8151e0b..114b15ac5 100644 --- a/tests/logger_loguru/test_attributes.py +++ b/tests/logger_loguru/test_attributes.py @@ -53,7 +53,7 @@ def _patcher(d): bound_logger.error("context_attrs: {}", "arg1", kwarg_attr=4) -@validate_log_events([{"message": "exc_info"}], required_attrs=["context.exception"]) +@validate_log_events([{"message": "exc_info"}], required_attrs=["context.file"]) @validate_log_event_count(1) @background_task() def test_loguru_exception_context_attributes(logger): From ac17284975182acd7d80345e50f80179636feae5 Mon Sep 17 00:00:00 2001 From: Hannah Stepanek Date: Fri, 16 Feb 2024 09:53:28 -0800 Subject: [PATCH 7/9] Fixup formatting of blank lines --- tests/agent_features/test_custom_events.py | 97 ++++++++++++++-------- 1 file changed, 61 insertions(+), 36 deletions(-) diff --git a/tests/agent_features/test_custom_events.py b/tests/agent_features/test_custom_events.py index afc58da51..c4da7d69d 100644 --- a/tests/agent_features/test_custom_events.py +++ b/tests/agent_features/test_custom_events.py @@ -14,51 +14,60 @@ import time +from testing_support.fixtures import ( + function_not_called, + override_application_settings, + reset_core_stats_engine, + validate_custom_event_count, + validate_custom_event_in_application_stats_engine, +) + from newrelic.api.application import application_instance as application from newrelic.api.background_task import background_task from newrelic.api.transaction import record_custom_event from newrelic.core.custom_event import process_event_type -from testing_support.fixtures import (reset_core_stats_engine, - validate_custom_event_count, - validate_custom_event_in_application_stats_engine, - override_application_settings, function_not_called) - -# Test process_event_type() def test_process_event_type_name_is_string(): - name = 'string' + name = "string" assert process_event_type(name) == name + def test_process_event_type_name_is_not_string(): name = 42 assert process_event_type(name) is None + def test_process_event_type_name_ok_length(): - ok_name = 'CustomEventType' + ok_name = "CustomEventType" assert process_event_type(ok_name) == ok_name + def test_process_event_type_name_too_long(): - too_long = 'a' * 256 + too_long = "a" * 256 assert process_event_type(too_long) is None + def test_process_event_type_name_valid_chars(): - valid_name = 'az09: ' + valid_name = "az09: " assert process_event_type(valid_name) == valid_name + def test_process_event_type_name_invalid_chars(): - invalid_name = '&' + invalid_name = "&" assert process_event_type(invalid_name) is None + _now = time.time() _intrinsics = { - 'type': 'FooEvent', - 'timestamp': _now, + "type": "FooEvent", + "timestamp": _now, } -_user_params = {'foo': 'bar'} +_user_params = {"foo": "bar"} _event = [_intrinsics, _user_params] + @reset_core_stats_engine() @validate_custom_event_in_application_stats_engine(_event) @background_task() @@ -82,79 +91,93 @@ def test_add_custom_event_to_application_stats_engine_drops_none_attr(): @validate_custom_event_in_application_stats_engine(_event) @background_task() def test_add_custom_event_to_transaction_stats_engine(): - record_custom_event('FooEvent', _user_params) + record_custom_event("FooEvent", _user_params) + @reset_core_stats_engine() @validate_custom_event_in_application_stats_engine(_event) def test_add_custom_event_to_application_stats_engine(): app = application() - record_custom_event('FooEvent', _user_params, application=app) + record_custom_event("FooEvent", _user_params, application=app) + @reset_core_stats_engine() @validate_custom_event_count(count=0) @background_task() def test_custom_event_inside_transaction_bad_event_type(): - record_custom_event('!@#$%^&*()', {'foo': 'bar'}) + record_custom_event("!@#$%^&*()", {"foo": "bar"}) + @reset_core_stats_engine() @validate_custom_event_count(count=0) @background_task() def test_custom_event_outside_transaction_bad_event_type(): app = application() - record_custom_event('!@#$%^&*()', {'foo': 'bar'}, application=app) + record_custom_event("!@#$%^&*()", {"foo": "bar"}, application=app) + + +_mixed_params = {"foo": "bar", 123: "bad key"} -_mixed_params = {'foo': 'bar', 123: 'bad key'} @reset_core_stats_engine() @validate_custom_event_in_application_stats_engine(_event) @background_task() def test_custom_event_inside_transaction_mixed_params(): - record_custom_event('FooEvent', _mixed_params) + record_custom_event("FooEvent", _mixed_params) + @reset_core_stats_engine() @validate_custom_event_in_application_stats_engine(_event) @background_task() def test_custom_event_outside_transaction_mixed_params(): app = application() - record_custom_event('FooEvent', _mixed_params, application=app) + record_custom_event("FooEvent", _mixed_params, application=app) + + +_bad_params = {"*" * 256: "too long", 123: "bad key"} +_event_with_no_params = [{"type": "FooEvent", "timestamp": _now}, {}] -_bad_params = {'*' * 256: 'too long', 123: 'bad key'} -_event_with_no_params = [{'type': 'FooEvent', 'timestamp': _now}, {}] @reset_core_stats_engine() @validate_custom_event_in_application_stats_engine(_event_with_no_params) @background_task() def test_custom_event_inside_transaction_bad_params(): - record_custom_event('FooEvent', _bad_params) + record_custom_event("FooEvent", _bad_params) + @reset_core_stats_engine() @validate_custom_event_in_application_stats_engine(_event_with_no_params) @background_task() def test_custom_event_outside_transaction_bad_params(): app = application() - record_custom_event('FooEvent', _bad_params, application=app) + record_custom_event("FooEvent", _bad_params, application=app) + @reset_core_stats_engine() @validate_custom_event_count(count=0) @background_task() def test_custom_event_params_not_a_dict(): - record_custom_event('ParamsListEvent', ['not', 'a', 'dict']) + record_custom_event("ParamsListEvent", ["not", "a", "dict"]) + # Tests for Custom Events configuration settings -@override_application_settings({'collect_custom_events': False}) + +@override_application_settings({"collect_custom_events": False}) @reset_core_stats_engine() @validate_custom_event_count(count=0) @background_task() def test_custom_event_settings_check_collector_flag(): - record_custom_event('FooEvent', _user_params) + record_custom_event("FooEvent", _user_params) + -@override_application_settings({'custom_insights_events.enabled': False}) +@override_application_settings({"custom_insights_events.enabled": False}) @reset_core_stats_engine() @validate_custom_event_count(count=0) @background_task() def test_custom_event_settings_check_custom_insights_enabled(): - record_custom_event('FooEvent', _user_params) + record_custom_event("FooEvent", _user_params) + # Test that record_custom_event() methods will short-circuit. # @@ -162,15 +185,17 @@ def test_custom_event_settings_check_custom_insights_enabled(): # `create_custom_event()` function is not called, in order to avoid the # event_type and attribute processing. -@override_application_settings({'custom_insights_events.enabled': False}) -@function_not_called('newrelic.api.transaction', 'create_custom_event') + +@override_application_settings({"custom_insights_events.enabled": False}) +@function_not_called("newrelic.api.transaction", "create_custom_event") @background_task() def test_transaction_create_custom_event_not_called(): - record_custom_event('FooEvent', _user_params) + record_custom_event("FooEvent", _user_params) + -@override_application_settings({'custom_insights_events.enabled': False}) -@function_not_called('newrelic.core.application', 'create_custom_event') +@override_application_settings({"custom_insights_events.enabled": False}) +@function_not_called("newrelic.core.application", "create_custom_event") @background_task() def test_application_create_custom_event_not_called(): app = application() - record_custom_event('FooEvent', _user_params, application=app) + record_custom_event("FooEvent", _user_params, application=app) From 27552081b66c258eefa20dcc65ee3f692144038e Mon Sep 17 00:00:00 2001 From: Hannah Stepanek Date: Tue, 20 Feb 2024 17:11:26 -0800 Subject: [PATCH 8/9] Summarize agent spec quote --- newrelic/core/attribute.py | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/newrelic/core/attribute.py b/newrelic/core/attribute.py index 77cc83feb..42c47bc6d 100644 --- a/newrelic/core/attribute.py +++ b/newrelic/core/attribute.py @@ -333,20 +333,9 @@ def sanitize(value): """ valid_value_types = (six.text_type, six.binary_type, bool, float, six.integer_types) - # The agent spec says: - # Agents **SHOULD NOT** report `null` attribute values. The behavior of `IS NULL` - # queries in insights makes it so that omitted keys behave the same as `null` - # keys. Since there is no difference between omitting the key and sending a - # `null, we **SHOULD** reduce the payload size by omitting `null` values from the - # payload entirely. - # - # Since there should not be a difference in behavior for customers recording - # `null` attributes versus customers omitting an attribute, agents **SHOULD** add - # debug logging when a null value is recorded. This will give agents observability - # on recording of `null` attributes outside of audit logs. - # - # "empty" values such as `""` and `0` **MUST** be sent as an attribute in the - # payload. + # According to the agent spec, agents should not report None attribute values. + # There is no difference between omitting the key and sending a None, so we can + # reduce the payload size by not sending None values. if value is None: raise NullValueException( "Attribute value is of type: None. Omitting value since there is " From 5bbf0dc1a187a1aceaeea0e77b18f7e405b860e4 Mon Sep 17 00:00:00 2001 From: Tim Pansino Date: Thu, 22 Feb 2024 12:18:20 -0800 Subject: [PATCH 9/9] Fix loguru test --- tests/logger_loguru/test_attributes.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/logger_loguru/test_attributes.py b/tests/logger_loguru/test_attributes.py index 114b15ac5..e15cc9aec 100644 --- a/tests/logger_loguru/test_attributes.py +++ b/tests/logger_loguru/test_attributes.py @@ -53,14 +53,14 @@ def _patcher(d): bound_logger.error("context_attrs: {}", "arg1", kwarg_attr=4) -@validate_log_events([{"message": "exc_info"}], required_attrs=["context.file"]) +@validate_log_events([{"message": "exc_info"}], required_attrs=["context.exception"]) @validate_log_event_count(1) @background_task() def test_loguru_exception_context_attributes(logger): try: raise RuntimeError("Oops") except Exception: - logger.error("exc_info") + logger.opt(exception=True).error("exc_info") @validate_log_events([{"context.extra.attr": 1}])