From 0a5a4de05a0b27c4ed1311d35d9f810c7456cf04 Mon Sep 17 00:00:00 2001 From: Thomas Pierce Date: Mon, 12 Feb 2024 14:17:52 -0800 Subject: [PATCH] Remove `set_http_status` logic from AwsMetricAttributeGenerator (#51) In this commit, we are removing the `set_http_status` logic from AwsMetricAttributeGenerator. This logic was originally in place for parity with Java. In Java, the OTEL instrumentation for AWS SDK does not populate the `http.status_code` attribute for spans, so we had to add logic to extract status code from the exception embedded within the events of the span. However, in Python, we have verified that this is not a problem; `http.status_code` is reliably set and so we do not need this logic. `set_http_status` logic is no longer needed, so this commit seeks to clean up the code by removing it and all code depending on it. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. --- .../distro/_aws_metric_attribute_generator.py | 19 ----------- .../distro/aws_span_metrics_processor.py | 3 -- .../test_aws_metric_attribute_generator.py | 2 -- .../distro/test_aws_span_metrics_processor.py | 33 ------------------- 4 files changed, 57 deletions(-) diff --git a/aws-opentelemetry-distro/src/amazon/opentelemetry/distro/_aws_metric_attribute_generator.py b/aws-opentelemetry-distro/src/amazon/opentelemetry/distro/_aws_metric_attribute_generator.py index 55be7678f..36a3b9ebd 100644 --- a/aws-opentelemetry-distro/src/amazon/opentelemetry/distro/_aws_metric_attribute_generator.py +++ b/aws-opentelemetry-distro/src/amazon/opentelemetry/distro/_aws_metric_attribute_generator.py @@ -43,7 +43,6 @@ _FAAS_TRIGGER: str = SpanAttributes.FAAS_TRIGGER _GRAPHQL_OPERATION_TYPE: str = SpanAttributes.GRAPHQL_OPERATION_TYPE _HTTP_METHOD: str = SpanAttributes.HTTP_METHOD -_HTTP_STATUS_CODE: str = SpanAttributes.HTTP_STATUS_CODE _HTTP_URL: str = SpanAttributes.HTTP_URL _MESSAGING_OPERATION: str = SpanAttributes.MESSAGING_OPERATION _MESSAGING_SYSTEM: str = SpanAttributes.MESSAGING_SYSTEM @@ -92,7 +91,6 @@ def _generate_service_metric_attributes(span: ReadableSpan, resource: Resource) _set_service(resource, span, attributes) _set_ingress_operation(span, attributes) _set_span_kind_for_service(span, attributes) - _set_http_status(span, attributes) return attributes @@ -103,7 +101,6 @@ def _generate_dependency_metric_attributes(span: ReadableSpan, resource: Resourc _set_remote_service_and_operation(span, attributes) _set_remote_target(span, attributes) _set_span_kind_for_dependency(span, attributes) - _set_http_status(span, attributes) return attributes @@ -140,22 +137,6 @@ def _set_span_kind_for_service(span: ReadableSpan, attributes: BoundedAttributes attributes[AWS_SPAN_KIND] = span_kind -def _set_http_status(span: ReadableSpan, attributes: BoundedAttributes) -> None: - if is_key_present(span, _HTTP_STATUS_CODE): - return - - status_code: Optional[int] = _get_aws_status_code(span) - if status_code is not None: - attributes[_HTTP_STATUS_CODE] = status_code - - -def _get_aws_status_code(span: ReadableSpan) -> Optional[int]: - """ - TODO: We need to determine if the same status code issue in Java AWS SDK instrumentation is present in Python. - """ - return None - - def _set_egress_operation(span: ReadableSpan, attributes: BoundedAttributes) -> None: """ Egress operation (i.e. operation for Client and Producer spans) is always derived from a special span attribute, diff --git a/aws-opentelemetry-distro/src/amazon/opentelemetry/distro/aws_span_metrics_processor.py b/aws-opentelemetry-distro/src/amazon/opentelemetry/distro/aws_span_metrics_processor.py index cfa8e7db6..78346748c 100644 --- a/aws-opentelemetry-distro/src/amazon/opentelemetry/distro/aws_span_metrics_processor.py +++ b/aws-opentelemetry-distro/src/amazon/opentelemetry/distro/aws_span_metrics_processor.py @@ -94,9 +94,6 @@ def _record_error_or_fault(self, span: ReadableSpan, attributes: BoundedAttribut http_status_code: int = span.attributes.get(_HTTP_STATUS_CODE) status_code: StatusCode = span.status.status_code - if http_status_code is None: - http_status_code = attributes.get(_HTTP_STATUS_CODE) - if _is_not_error_or_fault(http_status_code): if StatusCode.ERROR == status_code: self._error_histogram.record(0, attributes) diff --git a/aws-opentelemetry-distro/tests/amazon/opentelemetry/distro/test_aws_metric_attribute_generator.py b/aws-opentelemetry-distro/tests/amazon/opentelemetry/distro/test_aws_metric_attribute_generator.py index 1067910c2..46ddbc4eb 100644 --- a/aws-opentelemetry-distro/tests/amazon/opentelemetry/distro/test_aws_metric_attribute_generator.py +++ b/aws-opentelemetry-distro/tests/amazon/opentelemetry/distro/test_aws_metric_attribute_generator.py @@ -500,8 +500,6 @@ def test_peer_service_does_not_override_aws_remote_service(self): ).get(DEPENDENCY_METRIC) self.assertEqual(actual_attributes.get(AWS_REMOTE_SERVICE), "TestString") - # TODO: add HTTP_STATUS_CODE based test when http-status-code related implementation ready - def test_no_metric_when_consumer_process_with_consumer_parent(self): self._mock_attribute( [AWS_CONSUMER_PARENT_SPAN_KIND, SpanAttributes.MESSAGING_OPERATION], diff --git a/aws-opentelemetry-distro/tests/amazon/opentelemetry/distro/test_aws_span_metrics_processor.py b/aws-opentelemetry-distro/tests/amazon/opentelemetry/distro/test_aws_span_metrics_processor.py index 9d2f909bc..0b746903d 100644 --- a/aws-opentelemetry-distro/tests/amazon/opentelemetry/distro/test_aws_span_metrics_processor.py +++ b/aws-opentelemetry-distro/tests/amazon/opentelemetry/distro/test_aws_span_metrics_processor.py @@ -198,18 +198,6 @@ def test_on_end_metrics_generation_with_latency(self): self.fault_histogram_mock.assert_has_calls([call.record(0, metric_attributes_dict.get(SERVICE_METRIC))]) self.latency_histogram_mock.assert_has_calls([call.record(5.5, metric_attributes_dict.get(SERVICE_METRIC))]) - def test_on_end_metrics_generation_with_aws_status_codes(self): - # INVALID HTTP STATUS CODE - self._validate_metrics_generated_for_attributes_status_code(None, self.ExpectedStatusMetric.NEITHER) - - # VALID HTTP STATUS CODE - self._validate_metrics_generated_for_attributes_status_code(399, self.ExpectedStatusMetric.NEITHER) - self._validate_metrics_generated_for_attributes_status_code(400, self.ExpectedStatusMetric.ERROR) - self._validate_metrics_generated_for_attributes_status_code(499, self.ExpectedStatusMetric.ERROR) - self._validate_metrics_generated_for_attributes_status_code(500, self.ExpectedStatusMetric.FAULT) - self._validate_metrics_generated_for_attributes_status_code(599, self.ExpectedStatusMetric.FAULT) - self._validate_metrics_generated_for_attributes_status_code(600, self.ExpectedStatusMetric.NEITHER) - def test_on_end_metrics_generation_with_http_status_codes(self): # INVALID HTTP STATUS CODE self._validate_metrics_generated_for_http_status_code(None, self.ExpectedStatusMetric.NEITHER) @@ -317,27 +305,6 @@ def _validate_metrics_generated_for_http_status_code( self.aws_span_metrics_processor.on_end(span) self._valid_metrics(metric_attributes_dict, expected_status_metric) - def _validate_metrics_generated_for_attributes_status_code( - self, aws_status_code, expected_status_metric: ExpectedStatusMetric - ): - attributes: Attributes = {"new key": "new value"} - span: ReadableSpan = _build_readable_span_mock(attributes, SpanKind.PRODUCER) - metric_attributes_dict = _build_metric_attributes(_CONTAINS_ATTRIBUTES, span) - if aws_status_code is not None: - attr_temp_service = { - "new service key": "new service value", - SpanAttributes.HTTP_STATUS_CODE: aws_status_code, - } - metric_attributes_dict[SERVICE_METRIC] = attr_temp_service - attr_temp_dependency = { - "new dependency key": "new dependency value", - SpanAttributes.HTTP_STATUS_CODE: aws_status_code, - } - metric_attributes_dict[DEPENDENCY_METRIC] = attr_temp_dependency - self._configure_mock_for_on_end(span, metric_attributes_dict) - self.aws_span_metrics_processor.on_end(span) - self._valid_metrics(metric_attributes_dict, expected_status_metric) - def _valid_metrics(self, metric_attributes_dict, expected_status_metric: ExpectedStatusMetric): if expected_status_metric == self.ExpectedStatusMetric.ERROR: