Skip to content

Commit

Permalink
Merge pull request from GHSA-5rv5-6h4r-h22v
Browse files Browse the repository at this point in the history
* Fix unbound cardinality for label http_method in wsgi based middlewares

* cr: rename file

* cr: change label UNKNOWN to NONSTANDARD

* Update instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py

---------

Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>
  • Loading branch information
programmer04 and ocelotl committed Aug 15, 2023
1 parent 1beab82 commit 6007e0c
Show file tree
Hide file tree
Showing 5 changed files with 162 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@
OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS,
OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST,
OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE,
OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE,
OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS,
get_excluded_urls,
)

Expand Down Expand Up @@ -326,6 +328,25 @@ def test_flask_metric_values(self):
if isinstance(point, NumberDataPoint):
self.assertEqual(point.value, 0)

def _assert_basic_metric(self, expected_duration_attributes, expected_requests_count_attributes):
metrics_list = self.memory_metrics_reader.get_metrics_data()
for resource_metric in metrics_list.resource_metrics:
for scope_metrics in resource_metric.scope_metrics:
for metric in scope_metrics.metrics:
for point in list(metric.data.data_points):
if isinstance(point, HistogramDataPoint):
self.assertDictEqual(
expected_duration_attributes,
dict(point.attributes),
)
self.assertEqual(point.count, 1)
elif isinstance(point, NumberDataPoint):
self.assertDictEqual(
expected_requests_count_attributes,
dict(point.attributes),
)
self.assertEqual(point.value, 0)

def test_basic_metric_success(self):
self.client.get("/hello/756")
expected_duration_attributes = {
Expand All @@ -344,23 +365,62 @@ def test_basic_metric_success(self):
"http.flavor": "1.1",
"http.server_name": "localhost",
}
metrics_list = self.memory_metrics_reader.get_metrics_data()
for resource_metric in metrics_list.resource_metrics:
for scope_metrics in resource_metric.scope_metrics:
for metric in scope_metrics.metrics:
for point in list(metric.data.data_points):
if isinstance(point, HistogramDataPoint):
self.assertDictEqual(
expected_duration_attributes,
dict(point.attributes),
)
self.assertEqual(point.count, 1)
elif isinstance(point, NumberDataPoint):
self.assertDictEqual(
expected_requests_count_attributes,
dict(point.attributes),
)
self.assertEqual(point.value, 0)
self._assert_basic_metric(
expected_duration_attributes,
expected_requests_count_attributes,
)

def test_basic_metric_nonstandard_http_method_success(self):
self.client.open("/hello/756", method="NONSTANDARD")
expected_duration_attributes = {
"http.method": "UNKNOWN",
"http.host": "localhost",
"http.scheme": "http",
"http.flavor": "1.1",
"http.server_name": "localhost",
"net.host.port": 80,
"http.status_code": 405,
}
expected_requests_count_attributes = {
"http.method": "UNKNOWN",
"http.host": "localhost",
"http.scheme": "http",
"http.flavor": "1.1",
"http.server_name": "localhost",
}
self._assert_basic_metric(
expected_duration_attributes,
expected_requests_count_attributes,
)

@patch.dict(
"os.environ",
{
OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS: "1",
},
)
def test_basic_metric_nonstandard_http_method_allowed_success(self):
self.client.open("/hello/756", method="NONSTANDARD")
expected_duration_attributes = {
"http.method": "NONSTANDARD",
"http.host": "localhost",
"http.scheme": "http",
"http.flavor": "1.1",
"http.server_name": "localhost",
"net.host.port": 80,
"http.status_code": 405,
}
expected_requests_count_attributes = {
"http.method": "NONSTANDARD",
"http.host": "localhost",
"http.scheme": "http",
"http.flavor": "1.1",
"http.server_name": "localhost",
}
self._assert_basic_metric(
expected_duration_attributes,
expected_requests_count_attributes,
)

def test_metric_uninstrument(self):
self.client.delete("/hello/756")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,12 @@ def response_hook(span: Span, environ: WSGIEnvironment, status: str, response_he
Note:
The environment variable names used to capture HTTP headers are still experimental, and thus are subject to change.
Sanitizing methods
******************
In order to prevent unbound cardinality for HTTP methods by default nonstandard ones are labeled as ``NONSTANDARD``.
To record all of the names set the environment variable ``OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS``
to a value that evaluates to true, e.g. ``1``.
API
---
"""
Expand Down Expand Up @@ -226,6 +232,7 @@ def response_hook(span: Span, environ: WSGIEnvironment, status: str, response_he
normalise_request_header_name,
normalise_response_header_name,
remove_url_credentials,
sanitize_method,
)

_HTTP_VERSION_PREFIX = "HTTP/"
Expand Down Expand Up @@ -295,7 +302,7 @@ def collect_request_attributes(environ):
"""

result = {
SpanAttributes.HTTP_METHOD: environ.get("REQUEST_METHOD"),
SpanAttributes.HTTP_METHOD: sanitize_method(environ.get("REQUEST_METHOD")),
SpanAttributes.HTTP_SERVER_NAME: environ.get("SERVER_NAME"),
SpanAttributes.HTTP_SCHEME: environ.get("wsgi.url_scheme"),
}
Expand Down Expand Up @@ -450,7 +457,7 @@ def get_default_span_name(environ):
Returns:
The span name.
"""
method = environ.get("REQUEST_METHOD", "").strip()
method = sanitize_method(environ.get("REQUEST_METHOD", "").strip())
path = environ.get("PATH_INFO", "").strip()
if method and path:
return f"{method} {path}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SANITIZE_FIELDS,
OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST,
OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE,
OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS,
)


Expand Down Expand Up @@ -284,6 +285,24 @@ def test_wsgi_metrics(self):
)
self.assertTrue(number_data_point_seen and histogram_data_point_seen)

def test_nonstandard_http_method(self):
self.environ["REQUEST_METHOD"]= "NONSTANDARD"
app = otel_wsgi.OpenTelemetryMiddleware(simple_wsgi)
response = app(self.environ, self.start_response)
self.validate_response(response, span_name="HTTP UNKNOWN", http_method="UNKNOWN")

@mock.patch.dict(
"os.environ",
{
OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS: "1",
},
)
def test_nonstandard_http_method_allowed(self):
self.environ["REQUEST_METHOD"]= "NONSTANDARD"
app = otel_wsgi.OpenTelemetryMiddleware(simple_wsgi)
response = app(self.environ, self.start_response)
self.validate_response(response, span_name="HTTP NONSTANDARD", http_method="NONSTANDARD")

def test_default_span_name_missing_path_info(self):
"""Test that default span_names with missing path info."""
self.environ.pop("PATH_INFO")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@
"OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE"
)

OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS = (
"OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS"
)

# List of recommended metrics attributes
_duration_attrs = {
SpanAttributes.HTTP_METHOD,
Expand Down Expand Up @@ -186,6 +190,15 @@ def normalise_response_header_name(header: str) -> str:
key = header.lower().replace("-", "_")
return f"http.response.header.{key}"

def sanitize_method(method: str | None) -> str | None:
if method is None:
return None
method = method.upper()
if (environ.get(OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS) or
# Based on https://www.rfc-editor.org/rfc/rfc7231#section-4.1 and https://www.rfc-editor.org/rfc/rfc5789#section-2.
method in ["GET", "HEAD", "POST", "PUT", "DELETE", "CONNECT", "OPTIONS", "TRACE", "PATCH"]):
return method
return "NONSTANDARD"

def get_custom_headers(env_var: str) -> List[str]:
custom_headers = environ.get(env_var, [])
Expand Down
44 changes: 44 additions & 0 deletions util/opentelemetry-util-http/tests/test_sanitize_method.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# Copyright The OpenTelemetry Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import unittest
from unittest.mock import patch

from opentelemetry.util.http import (
OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS,
sanitize_method,
)

class TestSanitizeMethod(unittest.TestCase):
def test_standard_method_uppercase(self):
method = sanitize_method("GET")
self.assertEqual(method, "GET")

def test_standard_method_lowercase(self):
method = sanitize_method("get")
self.assertEqual(method, "GET")

def test_nonstandard_method(self):
method = sanitize_method("UNKNOWN")
self.assertEqual(method, "NONSTANDARD")

@patch.dict(
"os.environ",
{
OTEL_PYTHON_INSTRUMENTATION_HTTP_CAPTURE_ALL_METHODS: "1",
},
)
def test_nonstandard_method_allowed(self):
method = sanitize_method("UNKNOWN")
self.assertEqual(method, "NONSTANDARD")

0 comments on commit 6007e0c

Please sign in to comment.