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

Implement new HTTP semantic convention opt-in for Falcon #2790

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- `opentelemetry-instrumentation-kafka-python` Instrument temporary fork, kafka-python-ng
inside kafka-python's instrumentation
([#2537](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2537)))
- `opentelemetry-instrumentation-falcon` Implement new HTTP semantic convention opt-in for Falcon
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- `opentelemetry-instrumentation-falcon` Implement new HTTP semantic convention opt-in for Falcon
- `opentelemetry-instrumentation-falcon` Implement new HTTP semantic convention opt-in for Falcon
([#2790](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2790))


## Breaking changes

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,14 @@ def response_hook(span, req, resp):

import opentelemetry.instrumentation.wsgi as otel_wsgi
from opentelemetry import context, trace
from opentelemetry.instrumentation._semconv import (
_HTTPStabilityMode,
_OpenTelemetrySemanticConventionStability,
_OpenTelemetryStabilitySignalType,
_report_new,
_report_old,
_set_status,
)
from opentelemetry.instrumentation.falcon.package import _instruments
from opentelemetry.instrumentation.falcon.version import __version__
from opentelemetry.instrumentation.instrumentor import BaseInstrumentor
Expand All @@ -203,12 +211,12 @@ def response_hook(span, req, resp):
from opentelemetry.instrumentation.utils import (
_start_internal_or_server_span,
extract_attributes_from_object,
http_status_to_status_code,
)
from opentelemetry.metrics import get_meter
from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE
from opentelemetry.semconv.metrics import MetricInstruments
from opentelemetry.semconv.trace import SpanAttributes
from opentelemetry.trace.status import Status, StatusCode
from opentelemetry.trace.status import StatusCode
from opentelemetry.util.http import get_excluded_urls, get_traced_request_attrs

_logger = getLogger(__name__)
Expand Down Expand Up @@ -243,6 +251,11 @@ class _InstrumentedFalconAPI(getattr(falcon, _instrument_app)):
def __init__(self, *args, **kwargs):
otel_opts = kwargs.pop("_otel_opts", {})

_OpenTelemetrySemanticConventionStability._initialize()
Copy link
Member

@emdneto emdneto Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are missing some important points here:

Checklist

  • Add semconv status as migration for falcon in instrumentations README
  • Populate {method} as HTTP when nonstandard method (sanitize_method)
  • set request HTTP method attribute as _OTHER when nonstandard http method and for new semconv also set http.request.method_original with the original nonstandard method.
  • Set schema_url based on the opt-in mode
  • Create histograms based on the opt-in mode (different metric names so no dup attributes)
  • Set metrics attributes based on the opt-in mode
  • Set span attributes based on the opt-in mode
  • Tests for new_semconv, both_semconv and default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the great feedback, I'll get started on this.

self._sem_conv_opt_in_mode = _OpenTelemetrySemanticConventionStability._get_opentelemetry_stability_opt_in_mode(
_OpenTelemetryStabilitySignalType.HTTP,
)

# inject trace middleware
self._middlewares_list = kwargs.pop("middleware", [])
if self._middlewares_list is None:
Expand Down Expand Up @@ -283,6 +296,7 @@ def __init__(self, *args, **kwargs):
),
otel_opts.pop("request_hook", None),
otel_opts.pop("response_hook", None),
self._sem_conv_opt_in_mode,
)
self._middlewares_list.insert(0, trace_middleware)
kwargs["middleware"] = self._middlewares_list
Expand Down Expand Up @@ -382,9 +396,21 @@ def _start_response(status, response_headers, *args, **kwargs):
raise
finally:
if span.is_recording():
duration_attrs[SpanAttributes.HTTP_STATUS_CODE] = (
span.attributes.get(SpanAttributes.HTTP_STATUS_CODE)
)
if _report_old(self._sem_conv_opt_in_mode):
duration_attrs[SpanAttributes.HTTP_STATUS_CODE] = (
span.attributes.get(SpanAttributes.HTTP_STATUS_CODE)
)
if _report_new(self._sem_conv_opt_in_mode):
duration_attrs[
SpanAttributes.HTTP_RESPONSE_STATUS_CODE
] = span.attributes.get(
SpanAttributes.HTTP_RESPONSE_STATUS_CODE
)
if span.status.status_code == StatusCode.ERROR:
duration_attrs[ERROR_TYPE] = span.attributes.get(
ERROR_TYPE
)

duration = max(round((default_timer() - start) * 1000), 0)
self.duration_histogram.record(duration, duration_attrs)
self.active_requests_counter.add(-1, active_requests_count_attrs)
Expand All @@ -409,11 +435,13 @@ def __init__(
traced_request_attrs=None,
request_hook=None,
response_hook=None,
sem_conv_opt_in_mode: _HTTPStabilityMode = _HTTPStabilityMode.DEFAULT,
):
self.tracer = tracer
self._traced_request_attrs = traced_request_attrs
self._request_hook = request_hook
self._response_hook = response_hook
self._sem_conv_opt_in_mode = sem_conv_opt_in_mode

def process_request(self, req, resp):
span = req.env.get(_ENVIRON_SPAN_KEY)
Expand Down Expand Up @@ -446,10 +474,8 @@ def process_response(
return

status = resp.status
reason = None
if resource is None:
status = "404"
reason = "NotFound"
else:
if _ENVIRON_EXC in req.env:
exc = req.env[_ENVIRON_EXC]
Expand All @@ -459,28 +485,18 @@ def process_response(
if exc_type and not req_succeeded:
if "HTTPNotFound" in exc_type.__name__:
status = "404"
reason = "NotFound"
else:
status = "500"
reason = f"{exc_type.__name__}: {exc}"

status = status.split(" ")[0]
try:
status_code = int(status)
span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
otel_status_code = http_status_to_status_code(
status_code, server_span=True
)

# set the description only when the status code is ERROR
if otel_status_code is not StatusCode.ERROR:
reason = None

span.set_status(
Status(
status_code=otel_status_code,
description=reason,
)
_set_status(
span,
{},
int(status),
resp.status,
span.kind == trace.SpanKind.SERVER,
self._sem_conv_opt_in_mode,
)

# Falcon 1 does not support response headers. So
Expand All @@ -493,6 +509,9 @@ def process_response(
# Check if low-cardinality route is available as per semantic-conventions
if req.uri_template:
span.update_name(f"{req.method} {req.uri_template}")
span.set_attribute(
SpanAttributes.HTTP_ROUTE, req.uri_template
)
else:
span.update_name(f"{req.method}")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@

from opentelemetry import trace
from opentelemetry.instrumentation._semconv import (
_server_active_requests_count_attrs_new,
_server_active_requests_count_attrs_old,
_server_duration_attrs_new,
_server_duration_attrs_old,
)
from opentelemetry.instrumentation.falcon import FalconInstrumentor
Expand Down Expand Up @@ -53,8 +55,10 @@
"http.server.duration",
]
_recommended_attrs = {
"http.server.active_requests": _server_active_requests_count_attrs_old,
"http.server.duration": _server_duration_attrs_old,
"http.server.active_requests": _server_active_requests_count_attrs_new
+ _server_active_requests_count_attrs_old,
"http.server.duration": _server_duration_attrs_new
+ _server_duration_attrs_old,
}


Expand All @@ -66,6 +70,7 @@ def setUp(self):
{
"OTEL_PYTHON_FALCON_EXCLUDED_URLS": "ping",
"OTEL_PYTHON_FALCON_TRACED_REQUEST_ATTRS": "query_string",
"OTEL_SEMCONV_STABILITY_OPT_IN": "http/dup",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we missing the tests for "http" and "default"?

},
)
self.env_patch.start()
Expand Down Expand Up @@ -129,6 +134,7 @@ def _test_method(self, method):
SpanAttributes.HTTP_FLAVOR: "1.1",
"falcon.resource": "HelloWorldResource",
SpanAttributes.HTTP_STATUS_CODE: 201,
SpanAttributes.HTTP_ROUTE: "/hello",
},
)
# In falcon<3, NET_PEER_IP is always set by default to 127.0.0.1
Expand Down Expand Up @@ -180,10 +186,16 @@ def test_500(self):
self.assertEqual(span.name, "GET /error")
self.assertFalse(span.status.is_ok)
self.assertEqual(span.status.status_code, StatusCode.ERROR)
self.assertEqual(
span.status.description,
"NameError: name 'non_existent_var' is not defined",
)

_parsed_falcon_version = package_version.parse(_falcon_version)
if _parsed_falcon_version < package_version.parse("3.0.0"):
self.assertEqual(
span.status.description,
"NameError: name 'non_existent_var' is not defined",
)
else:
self.assertEqual(span.status.description, None)

self.assertSpanHasAttributes(
span,
{
Expand All @@ -196,6 +208,7 @@ def test_500(self):
SpanAttributes.NET_PEER_PORT: 65133,
SpanAttributes.HTTP_FLAVOR: "1.1",
SpanAttributes.HTTP_STATUS_CODE: 500,
SpanAttributes.HTTP_ROUTE: "/error",
},
)
# In falcon<3, NET_PEER_IP is always set by default to 127.0.0.1
Expand Down Expand Up @@ -230,6 +243,7 @@ def test_url_template(self):
SpanAttributes.HTTP_FLAVOR: "1.1",
"falcon.resource": "UserResource",
SpanAttributes.HTTP_STATUS_CODE: 200,
SpanAttributes.HTTP_ROUTE: "/user/{user_id}",
},
)

Expand Down Expand Up @@ -338,6 +352,7 @@ def test_falcon_metric_values(self):
"net.host.port": 80,
"net.host.name": "falconframework.org",
"http.status_code": 404,
"http.response.status_code": 404,
}
expected_requests_count_attributes = {
"http.method": "GET",
Expand Down