From e3f0ec8ee07107ef25155941f2b22727af270ef5 Mon Sep 17 00:00:00 2001 From: Kai Date: Sat, 19 Mar 2022 20:16:15 +0800 Subject: [PATCH] Handle binary and undefined response status and rename http.status to http.status_code in `opentelemetry-cowboy` (#48) * Handle binary resp_status from Cowboy and rename http.status to http.status_code * Fix test to use http.status_code as well * Handle resp_status could be undefined but not error - This could be due to websocket upgrade request. * Rename Status1 and transform_status to a more concise naming * Add test case for handling binary response code * Fix syntax and failing tests * Always convert cowboy status to status code * Set otel span status as error when status code >= 500 --- .../src/opentelemetry_cowboy.erl | 37 ++++++++++---- .../test/opentelemetry_cowboy_SUITE.erl | 51 +++++++++++++++---- .../opentelemetry_cowboy/test/test_h.erl | 2 + 3 files changed, 71 insertions(+), 19 deletions(-) diff --git a/instrumentation/opentelemetry_cowboy/src/opentelemetry_cowboy.erl b/instrumentation/opentelemetry_cowboy/src/opentelemetry_cowboy.erl index 5d8ce07..51a68af 100644 --- a/instrumentation/opentelemetry_cowboy/src/opentelemetry_cowboy.erl +++ b/instrumentation/opentelemetry_cowboy/src/opentelemetry_cowboy.erl @@ -56,16 +56,24 @@ handle_event([cowboy, request, stop], Measurements, Meta, _Config) -> 'http.response_content_length' => maps:get(resp_body_length, Measurements) }, otel_span:set_attributes(Ctx, Attributes), - case Status of + StatusCode = transform_status_to_code(Status), + case StatusCode of undefined -> - {ErrorType, Error, Reason} = maps:get(error, Meta), - otel_span:add_events(Ctx, [opentelemetry:event(ErrorType, #{error => Error, reason => Reason})]), - otel_span:set_status(Ctx, opentelemetry:status(?OTEL_STATUS_ERROR, Reason)); - Status when Status >= 400 -> - otel_span:set_attribute(Ctx, 'http.status', Status), + case maps:get(error, Meta, undefined) of + {ErrorType, Error, Reason} -> + otel_span:add_events(Ctx, [opentelemetry:event(ErrorType, #{error => Error, reason => Reason})]), + otel_span:set_status(Ctx, opentelemetry:status(?OTEL_STATUS_ERROR, Reason)); + _ -> + % do nothing first as I'm unsure how should we handle this + ok + end; + StatusCode when StatusCode >= 500 -> + otel_span:set_attribute(Ctx, 'http.status_code', StatusCode), otel_span:set_status(Ctx, opentelemetry:status(?OTEL_STATUS_ERROR, <<"">>)); - Status when Status < 400 -> - otel_span:set_attribute(Ctx, 'http.status', Status) + StatusCode when StatusCode >= 400 -> + otel_span:set_attribute(Ctx, 'http.status_code', StatusCode); + StatusCode when StatusCode < 400 -> + otel_span:set_attribute(Ctx, 'http.status_code', StatusCode) end, otel_telemetry:end_telemetry_span(?TRACER_ID, Meta), otel_ctx:clear(); @@ -80,8 +88,9 @@ handle_event([cowboy, request, exception], Measurements, Meta, _Config) -> } = Meta, otel_span:record_exception(Ctx, Kind, Reason, Stacktrace, []), otel_span:set_status(Ctx, opentelemetry:status(?OTEL_STATUS_ERROR, <<"">>)), + StatusCode = transform_status_to_code(Status), otel_span:set_attributes(Ctx, #{ - 'http.status' => Status, + 'http.status_code' => StatusCode, 'http.request_content_length' => maps:get(req_body_length, Measurements), 'http.response_content_length' => maps:get(resp_body_length, Measurements) }), @@ -93,8 +102,9 @@ handle_event([cowboy, request, early_error], Measurements, Meta, _Config) -> reason := {ErrorType, Error, Reason}, resp_status := Status } = Meta, + StatusCode = transform_status_to_code(Status), Attributes = #{ - 'http.status' => Status, + 'http.status_code' => StatusCode, 'http.response_content_length' => maps:get(resp_body_length, Measurements) }, Ctx = otel_telemetry:start_telemetry_span(?TRACER_ID, <<"HTTP Error">>, Meta, #{attributes => Attributes}), @@ -103,6 +113,13 @@ handle_event([cowboy, request, early_error], Measurements, Meta, _Config) -> otel_telemetry:end_telemetry_span(?TRACER_ID, Meta), otel_ctx:clear(). +transform_status_to_code(Status) when is_binary(Status) -> + [CodeString | _Message] = string:split(Status, " "), + {Code, _Rest} = string:to_integer(CodeString), + Code; +transform_status_to_code(Status) -> + Status. + http_flavor(Req) -> case maps:get(version, Req, undefined) of 'HTTP/1.0' -> '1.0'; diff --git a/instrumentation/opentelemetry_cowboy/test/opentelemetry_cowboy_SUITE.erl b/instrumentation/opentelemetry_cowboy/test/opentelemetry_cowboy_SUITE.erl index 5e3120a..7bcc7a2 100644 --- a/instrumentation/opentelemetry_cowboy/test/opentelemetry_cowboy_SUITE.erl +++ b/instrumentation/opentelemetry_cowboy/test/opentelemetry_cowboy_SUITE.erl @@ -9,8 +9,6 @@ -include_lib("opentelemetry/include/otel_span.hrl"). -include_lib("opentelemetry_api/include/otel_tracer.hrl"). --define(assertListsMatch(List1, List2), ?assertEqual(lists:sort(List1), lists:sort(List2))). - all() -> [ successful_request, @@ -19,7 +17,8 @@ all() -> client_timeout_request, idle_timeout_request, chunk_timeout_request, - bad_request + bad_request, + binary_status_code_request ]. init_per_suite(Config) -> @@ -30,7 +29,9 @@ init_per_suite(Config) -> {"/chunked", test_h, chunked}, {"/chunked_slow", test_h, chunked_slow}, {"/slow", test_h, slow}, - {"/failure", test_h, failure} + {"/failure", test_h, failure}, + {"/binary_status_code", test_h, + binary_status_code} ]}]), {ok, _} = cowboy:start_clear(http, [{port, 8080}], #{ env => #{dispatch => Dispatch}, @@ -84,7 +85,7 @@ successful_request(_Config) -> 'http.user_agent' => <<"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:81.0) Gecko/20100101 Firefox/81.0">>, 'net.host.ip' => <<"127.0.0.1">>, 'net.transport' => 'IP.TCP', - 'http.status' => 200, + 'http.status_code' => 200, 'http.request_content_length' => 0, 'http.response_content_length' => 12}, ?assertMatch(ExpectedAttrs, otel_attributes:map(Attributes)) @@ -109,7 +110,7 @@ chunked_request(_Config) -> 'http.user_agent' => <<>>, 'net.host.ip' => <<"127.0.0.1">>, 'net.transport' => 'IP.TCP', - 'http.status' => 200, + 'http.status_code' => 200, 'http.request_content_length' => 0, 'http.response_content_length' => 14}, ?assertMatch(ExpectedAttrs, otel_attributes:map(Attributes)) @@ -136,7 +137,7 @@ failed_request(_Config) -> 'http.user_agent' => <<>>, 'net.host.ip' => <<"127.0.0.1">>, 'net.transport' => 'IP.TCP', - 'http.status' => 500, + 'http.status_code' => 500, 'http.request_content_length' => 0, 'http.response_content_length' => 0}, ?assertMatch(ExpectedAttrs, otel_attributes:map(Attributes)) @@ -222,7 +223,7 @@ chunk_timeout_request(_Config) -> 'http.user_agent' => <<>>, 'net.host.ip' => <<"127.0.0.1">>, 'net.transport' => 'IP.TCP', - 'http.status' => 200, + 'http.status_code' => 200, 'http.request_content_length' => 0, 'http.response_content_length' => 0}, ?assertMatch(ExpectedAttrs, otel_attributes:map(Attributes)) @@ -248,9 +249,41 @@ bad_request(_Config) -> ?assertMatch(ExpectedEventAttrs, otel_attributes:map(EventAttributes)), ?assertEqual(<<"HTTP Error">>, Name), ExpectedAttrs = #{ - 'http.status' => 501, + 'http.status_code' => 501, 'http.response_content_length' => 0}, ?assertMatch(ExpectedAttrs, otel_attributes:map(Attributes)) after 1000 -> ct:fail(bad_request) end. + +binary_status_code_request(_Config) -> + Headers = [ + {"traceparent", "00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01"}, + {"tracestate", "congo=t61rcWkgMzE"}, + {"x-forwarded-for", "203.0.133.195, 70.41.3.18, 150.172.238.178"}, + {"user-agent", "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:81.0) Gecko/20100101 Firefox/81.0"}], + {ok, {{_Version, 200, _ReasonPhrase}, _Headers, _Body}} = + httpc:request(get, {"http://localhost:8080/binary_status_code", Headers}, [], []), + receive + {span, #span{name=Name,attributes=Attributes,parent_span_id=ParentSpanId}} -> + ?assertEqual(<<"HTTP GET">>, Name), + ?assertEqual(13235353014750950193, ParentSpanId), + ExpectedAttrs = #{ + 'http.client_ip' => <<"203.0.133.195">>, + 'http.flavor' => '1.1', + 'http.host' => <<"localhost">>, + 'http.host.port' => 8080, + 'http.method' => <<"GET">>, + 'http.scheme' => <<"http">>, + 'http.target' => <<"/binary_status_code">>, + 'http.user_agent' => <<"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:81.0) Gecko/20100101 Firefox/81.0">>, + 'net.host.ip' => <<"127.0.0.1">>, + 'net.transport' => 'IP.TCP', + 'http.status_code' => 200, + 'http.request_content_length' => 0, + 'http.response_content_length' => 12 + }, + ?assertMatch(ExpectedAttrs, otel_attributes:map(Attributes)) + after + 1000 -> ct:fail(binary_status_code_request) + end. diff --git a/instrumentation/opentelemetry_cowboy/test/test_h.erl b/instrumentation/opentelemetry_cowboy/test/test_h.erl index d6d142c..7e7e894 100644 --- a/instrumentation/opentelemetry_cowboy/test/test_h.erl +++ b/instrumentation/opentelemetry_cowboy/test/test_h.erl @@ -7,6 +7,8 @@ init(_, failure) -> error(failure); init(Req, success = Opts) -> {ok, cowboy_req:reply(200, #{}, <<"Hello world!">>, Req), Opts}; +init(Req, binary_status_code = Opts) -> + {ok, cowboy_req:reply(<<"200 OK">>, #{}, <<"Hello world!">>, Req), Opts}; init(Req, slow = Opts) -> timer:sleep(200), {ok, cowboy_req:reply(200, #{}, <<"I'm slow">>, Req), Opts};