Skip to content
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

Fix http clients method attribute in case of non standard http methods #2726

Merged
merged 4 commits into from
Jul 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#2682](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2682))
- Populate `{method}` as `HTTP` on `_OTHER` methods from scope for `django` middleware
([#2714](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2714))
- `opentelemetry-instrumentation-httpx`, `opentelemetry-instrumentation-aiohttp-client`,
`opentelemetry-instrumentation-requests` Populate `{method}` as `HTTP` on `_OTHER` methods
([#2726](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2726))

### Fixed
- Handle `redis.exceptions.WatchError` as a non-error event in redis instrumentation
Expand Down Expand Up @@ -79,7 +82,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#2153](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2153))
- `opentelemetry-instrumentation-asgi` Removed `NET_HOST_NAME` AND `NET_HOST_PORT` from active requests count attribute
([#2610](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2610))
- `opentelemetry-instrumentation-asgi` Bugfix: Middleware did not set status code attribute on duration metrics for non-recording spans.
- `opentelemetry-instrumentation-asgi` Bugfix: Middleware did not set status code attribute on duration metrics for non-recording spans.
([#2627](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2627))


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,9 @@ def response_hook(span: Span, params: typing.Union[


def _get_span_name(method: str) -> str:
method = sanitize_method(method.upper().strip())
method = sanitize_method(method.strip())
if method == "_OTHER":
method = "HTTP"

return method


Expand Down Expand Up @@ -230,8 +229,8 @@ async def on_request_start(
trace_config_ctx.span = None
return

http_method = params.method
request_span_name = _get_span_name(http_method)
method = params.method
request_span_name = _get_span_name(method)
lzchen marked this conversation as resolved.
Show resolved Hide resolved
request_url = (
remove_url_credentials(trace_config_ctx.url_filter(params.url))
if callable(trace_config_ctx.url_filter)
Expand All @@ -241,8 +240,8 @@ async def on_request_start(
span_attributes = {}
_set_http_method(
span_attributes,
http_method,
request_span_name,
method,
sanitize_method(method),
sem_conv_opt_in_mode,
)
_set_http_url(span_attributes, request_url, sem_conv_opt_in_mode)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE
from opentelemetry.semconv.attributes.http_attributes import (
HTTP_REQUEST_METHOD,
HTTP_REQUEST_METHOD_ORIGINAL,
HTTP_RESPONSE_STATUS_CODE,
)
from opentelemetry.semconv.attributes.url_attributes import URL_FULL
Expand Down Expand Up @@ -503,6 +504,92 @@ async def request_handler(request):
]
)

def test_nonstandard_http_method(self):
trace_configs = [aiohttp_client.create_trace_config()]
app = HttpServerMock("nonstandard_method")

@app.route("/status/200", methods=["NONSTANDARD"])
def index():
return ("", 405, {})

url = "http://localhost:5000/status/200"

with app.run("localhost", 5000):
with self.subTest(url=url):

async def do_request(url):
async with aiohttp.ClientSession(
trace_configs=trace_configs,
) as session:
async with session.request("NONSTANDARD", url):
pass

loop = asyncio.get_event_loop()
loop.run_until_complete(do_request(url))

self.assert_spans(
[
(
"HTTP",
(StatusCode.ERROR, None),
{
SpanAttributes.HTTP_METHOD: "_OTHER",
SpanAttributes.HTTP_URL: url,
SpanAttributes.HTTP_STATUS_CODE: int(
HTTPStatus.METHOD_NOT_ALLOWED
),
},
)
]
)
self.memory_exporter.clear()

def test_nonstandard_http_method_new_semconv(self):
trace_configs = [
aiohttp_client.create_trace_config(
sem_conv_opt_in_mode=_HTTPStabilityMode.HTTP
)
]
app = HttpServerMock("nonstandard_method")

@app.route("/status/200", methods=["NONSTANDARD"])
def index():
return ("", 405, {})

url = "http://localhost:5000/status/200"

with app.run("localhost", 5000):
with self.subTest(url=url):

async def do_request(url):
async with aiohttp.ClientSession(
trace_configs=trace_configs,
) as session:
async with session.request("NONSTANDARD", url):
pass

loop = asyncio.get_event_loop()
loop.run_until_complete(do_request(url))

self.assert_spans(
[
(
"HTTP",
(StatusCode.ERROR, None),
{
HTTP_REQUEST_METHOD: "_OTHER",
URL_FULL: url,
HTTP_RESPONSE_STATUS_CODE: int(
HTTPStatus.METHOD_NOT_ALLOWED
),
HTTP_REQUEST_METHOD_ORIGINAL: "NONSTANDARD",
ERROR_TYPE: "405",
},
)
]
)
self.memory_exporter.clear()

def test_credential_removal(self):
trace_configs = [aiohttp_client.create_trace_config()]

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ class ResponseInfo(typing.NamedTuple):


def _get_default_span_name(method: str) -> str:
method = sanitize_method(method.upper().strip())
method = sanitize_method(method.strip())
if method == "_OTHER":
method = "HTTP"

Expand Down Expand Up @@ -326,12 +326,16 @@ def _apply_request_client_attributes_to_span(
span_attributes: dict,
url: typing.Union[str, URL, httpx.URL],
method_original: str,
span_name: str,
semconv: _HTTPStabilityMode,
):
url = httpx.URL(url)
# http semconv transition: http.method -> http.request.method
_set_http_method(span_attributes, method_original, span_name, semconv)
_set_http_method(
span_attributes,
method_original,
sanitize_method(method_original),
semconv,
)
# http semconv transition: http.url -> url.full
_set_http_url(span_attributes, str(url), semconv)

Expand Down Expand Up @@ -450,7 +454,6 @@ def handle_request(
span_attributes,
url,
method_original,
span_name,
self._sem_conv_opt_in_mode,
)

Expand Down Expand Up @@ -572,7 +575,6 @@ async def handle_async_request(self, *args, **kwargs) -> typing.Union[
span_attributes,
url,
method_original,
span_name,
self._sem_conv_opt_in_mode,
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE
from opentelemetry.semconv.attributes.http_attributes import (
HTTP_REQUEST_METHOD,
HTTP_REQUEST_METHOD_ORIGINAL,
HTTP_RESPONSE_STATUS_CODE,
)
from opentelemetry.semconv.attributes.network_attributes import (
Expand Down Expand Up @@ -217,6 +218,59 @@ def test_basic(self):
span, opentelemetry.instrumentation.httpx
)

def test_nonstandard_http_method(self):
respx.route(method="NONSTANDARD").mock(
return_value=httpx.Response(405)
)
self.perform_request(self.URL, method="NONSTANDARD")
span = self.assert_span()

self.assertIs(span.kind, trace.SpanKind.CLIENT)
self.assertEqual(span.name, "HTTP")
self.assertEqual(
span.attributes,
{
SpanAttributes.HTTP_METHOD: "_OTHER",
SpanAttributes.HTTP_URL: self.URL,
SpanAttributes.HTTP_STATUS_CODE: 405,
},
)

self.assertIs(span.status.status_code, trace.StatusCode.ERROR)

self.assertEqualSpanInstrumentationInfo(
span, opentelemetry.instrumentation.httpx
)

def test_nonstandard_http_method_new_semconv(self):
respx.route(method="NONSTANDARD").mock(
return_value=httpx.Response(405)
)
self.perform_request(self.URL, method="NONSTANDARD")
span = self.assert_span()

self.assertIs(span.kind, trace.SpanKind.CLIENT)
self.assertEqual(span.name, "HTTP")
self.assertEqual(
span.attributes,
{
HTTP_REQUEST_METHOD: "_OTHER",
URL_FULL: self.URL,
SERVER_ADDRESS: "mock",
NETWORK_PEER_ADDRESS: "mock",
HTTP_RESPONSE_STATUS_CODE: 405,
NETWORK_PROTOCOL_VERSION: "1.1",
ERROR_TYPE: "405",
HTTP_REQUEST_METHOD_ORIGINAL: "NONSTANDARD",
},
)

self.assertIs(span.status.status_code, trace.StatusCode.ERROR)

self.assertEqualSpanInstrumentationInfo(
span, opentelemetry.instrumentation.httpx
)

def test_basic_new_semconv(self):
url = "http://mock:8080/status/200"
respx.get(url).mock(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,13 +188,19 @@ def get_or_create_headers():

emdneto marked this conversation as resolved.
Show resolved Hide resolved
span_attributes = {}
_set_http_method(
span_attributes, method, span_name, sem_conv_opt_in_mode
span_attributes,
method,
sanitize_method(method),
sem_conv_opt_in_mode,
)
_set_http_url(span_attributes, url, sem_conv_opt_in_mode)

metric_labels = {}
_set_http_method(
metric_labels, method, span_name, sem_conv_opt_in_mode
metric_labels,
method,
sanitize_method(method),
sem_conv_opt_in_mode,
)

try:
Expand Down Expand Up @@ -365,7 +371,7 @@ def get_default_span_name(method):
Returns:
span name
"""
method = sanitize_method(method.upper().strip())
method = sanitize_method(method.strip())
if method == "_OTHER":
return "HTTP"
return method
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE
from opentelemetry.semconv.attributes.http_attributes import (
HTTP_REQUEST_METHOD,
HTTP_REQUEST_METHOD_ORIGINAL,
HTTP_RESPONSE_STATUS_CODE,
)
from opentelemetry.semconv.attributes.network_attributes import (
Expand Down Expand Up @@ -247,6 +248,48 @@ def test_basic_both_semconv(self):
span, opentelemetry.instrumentation.requests
)

@mock.patch("httpretty.http.HttpBaseClass.METHODS", ("NONSTANDARD",))
def test_nonstandard_http_method(self):
httpretty.register_uri("NONSTANDARD", self.URL, status=405)
session = requests.Session()
session.request("NONSTANDARD", self.URL)
span = self.assert_span()
self.assertIs(span.kind, trace.SpanKind.CLIENT)
self.assertEqual(span.name, "HTTP")
self.assertEqual(
span.attributes,
{
SpanAttributes.HTTP_METHOD: "_OTHER",
SpanAttributes.HTTP_URL: self.URL,
SpanAttributes.HTTP_STATUS_CODE: 405,
},
)

self.assertIs(span.status.status_code, trace.StatusCode.ERROR)

@mock.patch("httpretty.http.HttpBaseClass.METHODS", ("NONSTANDARD",))
def test_nonstandard_http_method_new_semconv(self):
httpretty.register_uri("NONSTANDARD", self.URL, status=405)
session = requests.Session()
session.request("NONSTANDARD", self.URL)
span = self.assert_span()
self.assertIs(span.kind, trace.SpanKind.CLIENT)
self.assertEqual(span.name, "HTTP")
self.assertEqual(
span.attributes,
{
HTTP_REQUEST_METHOD: "_OTHER",
URL_FULL: self.URL,
SERVER_ADDRESS: "mock",
NETWORK_PEER_ADDRESS: "mock",
HTTP_RESPONSE_STATUS_CODE: 405,
NETWORK_PROTOCOL_VERSION: "1.1",
ERROR_TYPE: "405",
HTTP_REQUEST_METHOD_ORIGINAL: "NONSTANDARD",
},
)
self.assertIs(span.status.status_code, trace.StatusCode.ERROR)

def test_hooks(self):
def request_hook(span, request_obj):
span.update_name("name set from hook")
Expand Down