From 3489893ad3941f9c73171d5717dd9a01daa2320b Mon Sep 17 00:00:00 2001 From: Francis Bogsanyi Date: Tue, 8 Jun 2021 15:43:46 -0400 Subject: [PATCH 1/7] fix: total order constraint on span.status= --- sdk/lib/opentelemetry/sdk/trace/span.rb | 13 ++++++---- sdk/test/opentelemetry/sdk/trace/span_test.rb | 24 ++++++++++++++++++- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/sdk/lib/opentelemetry/sdk/trace/span.rb b/sdk/lib/opentelemetry/sdk/trace/span.rb index b100e2218..e2856a369 100644 --- a/sdk/lib/opentelemetry/sdk/trace/span.rb +++ b/sdk/lib/opentelemetry/sdk/trace/span.rb @@ -167,20 +167,23 @@ def record_exception(exception, attributes: nil) # Sets the Status to the Span # - # If used, this will override the default Span status. Default is OK. + # If used, this will override the default Span status. Default has code = Status::UNSET. # - # Only the value of the last call will be recorded, and implementations - # are free to ignore previous calls. + # An attempt to set the status with code == Status::UNSET is ignored. + # If the status is set with code == Status::OK, any further attempt to set the status + # is ignored. # # @param [Status] status The new status, which overrides the default Span - # status, which is OK. + # status, which has code = Status::UNSET. # # @return [void] def status=(status) + return if status.code == OpenTelemetry::Trace::Status::UNSET + @mutex.synchronize do if @ended OpenTelemetry.logger.warn('Calling status= on an ended Span.') - else + elsif @status.code != OpenTelemetry::Trace::Status::OK @status = status end end diff --git a/sdk/test/opentelemetry/sdk/trace/span_test.rb b/sdk/test/opentelemetry/sdk/trace/span_test.rb index 70ffad7e2..0fa0494cf 100644 --- a/sdk/test/opentelemetry/sdk/trace/span_test.rb +++ b/sdk/test/opentelemetry/sdk/trace/span_test.rb @@ -324,8 +324,9 @@ describe '#status=' do it 'sets the status' do - span.status = Status.new(1, description: 'cancelled') + span.status = Status.new(Status::ERROR, description: 'cancelled') _(span.status.description).must_equal('cancelled') + _(span.status.code).must_equal(Status::ERROR) end it 'does not set the status if span is ended' do @@ -333,6 +334,27 @@ span.status = Status.new(Status::ERROR, description: 'cancelled') _(span.status.code).must_equal(Status::UNSET) end + + it 'does not set the status if asked to set to UNSET' do + span.status = Status.new(Status::ERROR, description: 'cancelled') + span.status = Status.new(Status::UNSET, description: 'actually, maybe it is OK?') + _(span.status.description).must_equal('cancelled') + _(span.status.code).must_equal(Status::ERROR) + end + + it 'does not override the status once set to OK' do + span.status = Status.new(Status::OK, description: 'nothing to see here') + span.status = Status.new(Status::ERROR, description: 'cancelled') + _(span.status.description).must_equal('nothing to see here') + _(span.status.code).must_equal(Status::OK) + end + + it 'allows overriding ERROR with OK' do + span.status = Status.new(Status::ERROR, description: 'cancelled') + span.status = Status.new(Status::OK, description: 'nothing to see here') + _(span.status.description).must_equal('nothing to see here') + _(span.status.code).must_equal(Status::OK) + end end describe '#name=' do From 87c332a8f18cf7e4ed7a036c65c019857124b5a4 Mon Sep 17 00:00:00 2001 From: Francis Bogsanyi Date: Wed, 9 Jun 2021 23:25:55 -0400 Subject: [PATCH 2/7] fix!: remove http_to_status helper --- api/lib/opentelemetry/trace/status.rb | 39 ++++++++++++++++--- .../trace/util/http_to_status.rb | 28 ------------- api/test/opentelemetry/trace/status_test.rb | 35 ----------------- instrumentation/datadog-porting-guide.md | 1 - .../instrumentation/ethon/patches/easy.rb | 9 +---- .../excon/middlewares/tracer_middleware.rb | 4 +- .../faraday/middlewares/tracer_middleware.rb | 4 +- .../instrumentation/http/patches/client.rb | 2 +- .../http_client/patches/client.rb | 4 +- .../net/http/patches/instrumentation.rb | 4 +- .../rack/middlewares/tracer_middleware.rb | 2 +- .../restclient/patches/request.rb | 8 +--- .../sinatra/middlewares/tracer_middleware.rb | 2 +- 13 files changed, 45 insertions(+), 97 deletions(-) delete mode 100644 api/lib/opentelemetry/trace/util/http_to_status.rb diff --git a/api/lib/opentelemetry/trace/status.rb b/api/lib/opentelemetry/trace/status.rb index 8a342a8dd..e5a6bfbe5 100644 --- a/api/lib/opentelemetry/trace/status.rb +++ b/api/lib/opentelemetry/trace/status.rb @@ -4,15 +4,41 @@ # # SPDX-License-Identifier: Apache-2.0 -require 'opentelemetry/trace/util/http_to_status' - module OpenTelemetry module Trace # Status represents the status of a finished {Span}. It is composed of a # status code in conjunction with an optional descriptive message. class Status - # Convenience utility, not in API spec: - extend Util::HttpToStatus + class << self + private :new # rubocop:disable Style/AccessModifierDeclarations + + # Returns a newly created {Status} with code == UNSET and an optional + # description. + # + # @param [String] description + # @return [Status] + def unset(description = '') + new(UNSET, description: description) + end + + # Returns a newly created {Status} with code == OK and an optional + # description. + # + # @param [String] description + # @return [Status] + def ok(description = '') + new(OK, description: description) + end + + # Returns a newly created {Status} with code == ERROR and an optional + # description. + # + # @param [String] description + # @return [Status] + def error(description = '') + new(ERROR, description: description) + end + end # Retrieve the status code of this Status. # @@ -24,7 +50,10 @@ class Status # @return [String] attr_reader :description - # Initialize a Status. + # @api private + # The constructor is private and only for use internally by the class. + # Users should use the {unset}, {error}, or {ok} factory methods to + # obtain a {Status} instance. # # @param [Integer] code One of the status codes below # @param [String] description diff --git a/api/lib/opentelemetry/trace/util/http_to_status.rb b/api/lib/opentelemetry/trace/util/http_to_status.rb deleted file mode 100644 index 16422e54f..000000000 --- a/api/lib/opentelemetry/trace/util/http_to_status.rb +++ /dev/null @@ -1,28 +0,0 @@ -# frozen_string_literal: true - -# Copyright The OpenTelemetry Authors -# -# SPDX-License-Identifier: Apache-2.0 - -module OpenTelemetry - module Trace - module Util - # Convenience methods, not necessarily required by the API specification. - module HttpToStatus - # Maps numeric HTTP status codes to Trace::Status. This module is a mixin for Trace::Status - # and is not intended for standalone use. - # - # @param code Numeric HTTP status - # @return Status - def http_to_status(code) - case code.to_i - when 100..399 - new(const_get(:OK)) - else - new(const_get(:ERROR)) - end - end - end - end - end -end diff --git a/api/test/opentelemetry/trace/status_test.rb b/api/test/opentelemetry/trace/status_test.rb index 8a0cf4682..3e77bded9 100644 --- a/api/test/opentelemetry/trace/status_test.rb +++ b/api/test/opentelemetry/trace/status_test.rb @@ -9,41 +9,6 @@ describe OpenTelemetry::Trace::Status do let(:trace_status) { OpenTelemetry::Trace::Status } - describe '.http_to_status' do - it 'returns Status' do - _(trace_status.http_to_status(200)).must_be_kind_of trace_status - end - - def assert_http_to_status(http_code, trace_status_code) - _(trace_status.http_to_status(http_code).code).must_equal trace_status_code - end - - it 'maps http 1xx codes' do - assert_http_to_status(100, trace_status::OK) - assert_http_to_status(199, trace_status::OK) - end - - it 'maps http 2xx codes' do - assert_http_to_status(200, trace_status::OK) - assert_http_to_status(299, trace_status::OK) - end - - it 'maps http 3xx codes' do - assert_http_to_status(300, trace_status::OK) - assert_http_to_status(399, trace_status::OK) - end - - it 'maps http 4xx codes' do - assert_http_to_status(400, trace_status::ERROR) - assert_http_to_status(499, trace_status::ERROR) - end - - it 'maps http 5xx codes' do - assert_http_to_status(500, trace_status::ERROR) - assert_http_to_status(599, trace_status::ERROR) - end - end - describe '.code' do it 'reflects the value passed in' do status = OpenTelemetry::Trace::Status.new(0) diff --git a/instrumentation/datadog-porting-guide.md b/instrumentation/datadog-porting-guide.md index 657683ad0..dc9c63c75 100644 --- a/instrumentation/datadog-porting-guide.md +++ b/instrumentation/datadog-porting-guide.md @@ -32,7 +32,6 @@ bash-5.0$ ruby trace_demonstration.rb ## Implementation * It's ok to use `require_relative` for files that are internal to the project -* `Span#status` can be set via helper `OpenTelemetry::Trace::Status.http_to_status` * Don't load integration implementation (require file) until `install` ('patch')-time * Most times, only want to run installation (via `#install`) once, but need to consider being able to `reset` somehow for, e.g., testing diff --git a/instrumentation/ethon/lib/opentelemetry/instrumentation/ethon/patches/easy.rb b/instrumentation/ethon/lib/opentelemetry/instrumentation/ethon/patches/easy.rb index 342ff7770..dc4172ef2 100644 --- a/instrumentation/ethon/lib/opentelemetry/instrumentation/ethon/patches/easy.rb +++ b/instrumentation/ethon/lib/opentelemetry/instrumentation/ethon/patches/easy.rb @@ -39,15 +39,10 @@ def complete # rubocop:disable Metrics/MethodLength if response_code.zero? return_code = response_options[:return_code] message = return_code ? ::Ethon::Curl.easy_strerror(return_code) : 'unknown reason' - @otel_span.status = OpenTelemetry::Trace::Status.new( - OpenTelemetry::Trace::Status::ERROR, - description: "Request has failed: #{message}" - ) + @otel_span.status = OpenTelemetry::Trace::Status.error("Request has failed: #{message}") else @otel_span.set_attribute('http.status_code', response_code) - @otel_span.status = OpenTelemetry::Trace::Status.http_to_status( - response_code - ) + @otel_span.status = OpenTelemetry::Trace::Status.error unless (100..399) === response_code.to_i end ensure @otel_span.finish diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index adbe38e6c..8a29559b7 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb @@ -79,9 +79,7 @@ def handle_response(datum) # rubocop:disable Metrics/AbcSize, Metrics/MethodLeng if datum.key?(:response) response = datum[:response] span.set_attribute('http.status_code', response[:status]) - span.status = OpenTelemetry::Trace::Status.http_to_status( - response[:status] - ) + span.status = OpenTelemetry::Trace::Status.error unless (100..399) === response[:status].to_i end if datum.key?(:error) diff --git a/instrumentation/faraday/lib/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware.rb b/instrumentation/faraday/lib/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware.rb index a9892417b..6033f9aa9 100644 --- a/instrumentation/faraday/lib/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware.rb +++ b/instrumentation/faraday/lib/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware.rb @@ -58,9 +58,7 @@ def tracer def trace_response(span, response) span.set_attribute('http.status_code', response.status) - span.status = OpenTelemetry::Trace::Status.http_to_status( - response.status - ) + span.status = OpenTelemetry::Trace::Status.error unless (100..399) === response.status.to_i end end end diff --git a/instrumentation/http/lib/opentelemetry/instrumentation/http/patches/client.rb b/instrumentation/http/lib/opentelemetry/instrumentation/http/patches/client.rb index 643b3e313..d99e4aa5f 100644 --- a/instrumentation/http/lib/opentelemetry/instrumentation/http/patches/client.rb +++ b/instrumentation/http/lib/opentelemetry/instrumentation/http/patches/client.rb @@ -38,7 +38,7 @@ def annotate_span_with_response!(span, response) status_code = response.status.to_i span.set_attribute('http.status_code', status_code) - span.status = OpenTelemetry::Trace::Status.http_to_status(status_code) + span.status = OpenTelemetry::Trace::Status.error unless (100..399) === status_code.to_i end def tracer diff --git a/instrumentation/http_client/lib/opentelemetry/instrumentation/http_client/patches/client.rb b/instrumentation/http_client/lib/opentelemetry/instrumentation/http_client/patches/client.rb index fcbff994d..81f5358dc 100644 --- a/instrumentation/http_client/lib/opentelemetry/instrumentation/http_client/patches/client.rb +++ b/instrumentation/http_client/lib/opentelemetry/instrumentation/http_client/patches/client.rb @@ -41,9 +41,7 @@ def annotate_span_with_response!(span, response) status_code = response.status_code.to_i span.set_attribute('http.status_code', status_code) - span.status = OpenTelemetry::Trace::Status.http_to_status( - status_code - ) + span.status = OpenTelemetry::Trace::Status.error unless (100..399) === status_code.to_i end def tracer diff --git a/instrumentation/net_http/lib/opentelemetry/instrumentation/net/http/patches/instrumentation.rb b/instrumentation/net_http/lib/opentelemetry/instrumentation/net/http/patches/instrumentation.rb index b3d8ea9c3..9b46c88ec 100644 --- a/instrumentation/net_http/lib/opentelemetry/instrumentation/net/http/patches/instrumentation.rb +++ b/instrumentation/net_http/lib/opentelemetry/instrumentation/net/http/patches/instrumentation.rb @@ -64,9 +64,7 @@ def annotate_span_with_response!(span, response) status_code = response.code.to_i span.set_attribute('http.status_code', status_code) - span.status = OpenTelemetry::Trace::Status.http_to_status( - status_code - ) + span.status = OpenTelemetry::Trace::Status.error unless (100..399) === status_code.to_i end def tracer diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/tracer_middleware.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/tracer_middleware.rb index d4e52a7e6..a99fd2db5 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/tracer_middleware.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/tracer_middleware.rb @@ -153,7 +153,7 @@ def create_request_span_name(request_uri_or_path_info, env) end def set_attributes_after_request(span, status, headers, _response) - span.status = OpenTelemetry::Trace::Status.http_to_status(status) + span.status = OpenTelemetry::Trace::Status.error unless (100..399) === status.to_i span.set_attribute('http.status_code', status) # NOTE: if data is available, it would be good to do this: diff --git a/instrumentation/restclient/lib/opentelemetry/instrumentation/restclient/patches/request.rb b/instrumentation/restclient/lib/opentelemetry/instrumentation/restclient/patches/request.rb index 60339cee7..bada697fe 100644 --- a/instrumentation/restclient/lib/opentelemetry/instrumentation/restclient/patches/request.rb +++ b/instrumentation/restclient/lib/opentelemetry/instrumentation/restclient/patches/request.rb @@ -43,16 +43,12 @@ def trace_request # rubocop:disable Metrics/AbcSize, Metrics/MethodLength # If so, add additional attributes. if response.is_a?(::RestClient::Response) span.set_attribute('http.status_code', response.code) - span.status = OpenTelemetry::Trace::Status.http_to_status( - response.code - ) + span.status = OpenTelemetry::Trace::Status.error unless (100..399) === response.code.to_i end end rescue ::RestClient::ExceptionWithResponse => e span.set_attribute('http.status_code', e.http_code) - span.status = OpenTelemetry::Trace::Status.http_to_status( - e.http_code - ) + span.status = OpenTelemetry::Trace::Status.error unless (100..399) === e.http_code.to_i raise e ensure diff --git a/instrumentation/sinatra/lib/opentelemetry/instrumentation/sinatra/middlewares/tracer_middleware.rb b/instrumentation/sinatra/lib/opentelemetry/instrumentation/sinatra/middlewares/tracer_middleware.rb index 328981822..250e768f7 100644 --- a/instrumentation/sinatra/lib/opentelemetry/instrumentation/sinatra/middlewares/tracer_middleware.rb +++ b/instrumentation/sinatra/lib/opentelemetry/instrumentation/sinatra/middlewares/tracer_middleware.rb @@ -43,7 +43,7 @@ def trace_response(span, env, resp) span.set_attribute('http.status_code', status) span.set_attribute('http.route', env['sinatra.route'].split.last) if env['sinatra.route'] span.name = env['sinatra.route'] if env['sinatra.route'] - span.status = OpenTelemetry::Trace::Status.http_to_status(status) + span.status = OpenTelemetry::Trace::Status.error unless (100..399) === status.to_i end end end From 556253222bd246c902bab8bb375134bd837f6c38 Mon Sep 17 00:00:00 2001 From: Francis Bogsanyi Date: Wed, 9 Jun 2021 23:38:13 -0400 Subject: [PATCH 3/7] fix tests & other uses of Status.new --- api/lib/opentelemetry/trace/tracer.rb | 3 +-- api/test/opentelemetry/trace/status_test.rb | 25 +++++-------------- .../exporters/jaeger/encoder_test.rb | 2 +- .../exporter/otlp/exporter_test.rb | 2 +- .../exporters/zipkin/transformer_test.rb | 6 ++--- .../excon/middlewares/tracer_middleware.rb | 5 +--- .../instrumentation/redis/patches/client.rb | 5 +--- sdk/lib/opentelemetry/sdk/trace/span.rb | 2 +- sdk/test/opentelemetry/sdk/trace/span_test.rb | 16 ++++++------ 9 files changed, 23 insertions(+), 43 deletions(-) diff --git a/api/lib/opentelemetry/trace/tracer.rb b/api/lib/opentelemetry/trace/tracer.rb index a56d4ea15..d31ac284f 100644 --- a/api/lib/opentelemetry/trace/tracer.rb +++ b/api/lib/opentelemetry/trace/tracer.rb @@ -29,8 +29,7 @@ def in_span(name, attributes: nil, links: nil, start_timestamp: nil, kind: nil) Trace.with_span(span) { |s, c| yield s, c } rescue Exception => e # rubocop:disable Lint/RescueException span&.record_exception(e) - span&.status = Status.new(Status::ERROR, - description: "Unhandled exception of type: #{e.class}") + span&.status = Status.error("Unhandled exception of type: #{e.class}") raise e ensure span&.finish diff --git a/api/test/opentelemetry/trace/status_test.rb b/api/test/opentelemetry/trace/status_test.rb index 3e77bded9..372e6f434 100644 --- a/api/test/opentelemetry/trace/status_test.rb +++ b/api/test/opentelemetry/trace/status_test.rb @@ -11,45 +11,32 @@ describe '.code' do it 'reflects the value passed in' do - status = OpenTelemetry::Trace::Status.new(0) + status = OpenTelemetry::Trace::Status.ok _(status.code).must_equal(0) end end describe '.description' do it 'is an empty string by default' do - status = OpenTelemetry::Trace::Status.new(0) + status = OpenTelemetry::Trace::Status.ok _(status.description).must_equal('') end it 'reflects the value passed in' do - status = OpenTelemetry::Trace::Status.new(0, description: 'ok') + status = OpenTelemetry::Trace::Status.ok('ok') _(status.description).must_equal('ok') end end - describe '.initialize' do - it 'initializes a Status with required arguments' do - status = OpenTelemetry::Trace::Status.new(0, description: 'this is ok') - _(status.code).must_equal(0) - _(status.description).must_equal('this is ok') - end - end - describe '.ok?' do it 'reflects code when OK' do - ok = OpenTelemetry::Trace::Status::OK - status = OpenTelemetry::Trace::Status.new(ok) + status = OpenTelemetry::Trace::Status.ok _(status.ok?).must_equal(true) end it 'reflects code when not OK' do - codes = OpenTelemetry::Trace::Status.constants - %i[OK UNSET] - codes.each do |code| - code = OpenTelemetry::Trace::Status.const_get(code) - status = OpenTelemetry::Trace::Status.new(code) - _(status.ok?).must_equal(false) - end + status = OpenTelemetry::Trace::Status.error + _(status.ok?).must_equal(false) end end end diff --git a/exporter/jaeger/test/opentelemetry/exporters/jaeger/encoder_test.rb b/exporter/jaeger/test/opentelemetry/exporters/jaeger/encoder_test.rb index 8da77003c..6907e55b8 100644 --- a/exporter/jaeger/test/opentelemetry/exporters/jaeger/encoder_test.rb +++ b/exporter/jaeger/test/opentelemetry/exporters/jaeger/encoder_test.rb @@ -27,7 +27,7 @@ end it 'encodes span.status and span.kind' do - span_data = create_span_data(status: OpenTelemetry::Trace::Status.new(OpenTelemetry::Trace::Status::ERROR), kind: :server) + span_data = create_span_data(status: OpenTelemetry::Trace::Status.error, kind: :server) encoded_span = Encoder.encoded_span(span_data) _(encoded_span.tags.size).must_equal(2) diff --git a/exporter/otlp/test/opentelemetry/exporter/otlp/exporter_test.rb b/exporter/otlp/test/opentelemetry/exporter/otlp/exporter_test.rb index 04622ad64..5c68cbd46 100644 --- a/exporter/otlp/test/opentelemetry/exporter/otlp/exporter_test.rb +++ b/exporter/otlp/test/opentelemetry/exporter/otlp/exporter_test.rb @@ -274,7 +274,7 @@ span['i'] = 2 span['s'] = 'val' span['a'] = [3, 4] - span.status = OpenTelemetry::Trace::Status.new(OpenTelemetry::Trace::Status::ERROR) + span.status = OpenTelemetry::Trace::Status.error child_ctx = OpenTelemetry::Trace.context_with_span(span) client = with_ids(trace_id, client_span_id) { tracer.start_span('client', with_parent: child_ctx, kind: :client, start_timestamp: start_timestamp + 2).finish(end_timestamp: end_timestamp) } client_ctx = OpenTelemetry::Trace.context_with_span(client) diff --git a/exporter/zipkin/test/opentelemetry/exporters/zipkin/transformer_test.rb b/exporter/zipkin/test/opentelemetry/exporters/zipkin/transformer_test.rb index 77145c8b4..84da02951 100644 --- a/exporter/zipkin/test/opentelemetry/exporters/zipkin/transformer_test.rb +++ b/exporter/zipkin/test/opentelemetry/exporters/zipkin/transformer_test.rb @@ -18,7 +18,7 @@ it 'encodes span.status and span.kind' do resource = OpenTelemetry::SDK::Resources::Resource.create('service.name' => 'foo') - span_data = create_span_data(attributes: { 'bar' => 'baz' }, status: OpenTelemetry::Trace::Status.new(OpenTelemetry::Trace::Status::ERROR), kind: :server) + span_data = create_span_data(attributes: { 'bar' => 'baz' }, status: OpenTelemetry::Trace::Status.error, kind: :server) encoded_span = Transformer.to_zipkin_span(span_data, resource) @@ -82,7 +82,7 @@ describe 'status' do it 'encodes status code as strings' do - status = OpenTelemetry::Trace::Status.new(OpenTelemetry::Trace::Status::OK) + status = OpenTelemetry::Trace::Status.ok resource = OpenTelemetry::SDK::Resources::Resource.create('service.name' => 'foo') span_data = create_span_data(attributes: { 'bar' => 'baz' }, status: status) @@ -94,7 +94,7 @@ it 'encodes error status code as strings on error tag and status description field' do error_description = 'there is as yet insufficient data for a meaningful answer' - status = OpenTelemetry::Trace::Status.new(OpenTelemetry::Trace::Status::ERROR, description: error_description) + status = OpenTelemetry::Trace::Status.error(error_description) resource = OpenTelemetry::SDK::Resources::Resource.create('service.name' => 'foo') span_data = create_span_data(attributes: { 'bar' => 'baz' }, status: status) encoded_span = Transformer.to_zipkin_span(span_data, resource) diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index 8a29559b7..34894b4f2 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb @@ -83,10 +83,7 @@ def handle_response(datum) # rubocop:disable Metrics/AbcSize, Metrics/MethodLeng end if datum.key?(:error) - span.status = OpenTelemetry::Trace::Status.new( - OpenTelemetry::Trace::Status::ERROR, - description: "Request has failed: #{datum[:error]}" - ) + span.status = OpenTelemetry::Trace::Status.error("Request has failed: #{datum[:error]}") end span.finish diff --git a/instrumentation/redis/lib/opentelemetry/instrumentation/redis/patches/client.rb b/instrumentation/redis/lib/opentelemetry/instrumentation/redis/patches/client.rb index 4ebce7af3..567857e43 100644 --- a/instrumentation/redis/lib/opentelemetry/instrumentation/redis/patches/client.rb +++ b/instrumentation/redis/lib/opentelemetry/instrumentation/redis/patches/client.rb @@ -44,10 +44,7 @@ def process(commands) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, M super(commands).tap do |reply| if reply.is_a?(::Redis::CommandError) s.record_exception(reply) - s.status = Trace::Status.new( - Trace::Status::ERROR, - description: reply.message - ) + s.status = Trace::Status.error(reply.message) end end end diff --git a/sdk/lib/opentelemetry/sdk/trace/span.rb b/sdk/lib/opentelemetry/sdk/trace/span.rb index e2856a369..5e50ea29d 100644 --- a/sdk/lib/opentelemetry/sdk/trace/span.rb +++ b/sdk/lib/opentelemetry/sdk/trace/span.rb @@ -16,7 +16,7 @@ module Trace # # rubocop:disable Metrics/ClassLength class Span < OpenTelemetry::Trace::Span - DEFAULT_STATUS = OpenTelemetry::Trace::Status.new(OpenTelemetry::Trace::Status::UNSET) + DEFAULT_STATUS = OpenTelemetry::Trace::Status.unset EMPTY_ATTRIBUTES = {}.freeze private_constant :DEFAULT_STATUS, :EMPTY_ATTRIBUTES diff --git a/sdk/test/opentelemetry/sdk/trace/span_test.rb b/sdk/test/opentelemetry/sdk/trace/span_test.rb index 0fa0494cf..a1cae77c8 100644 --- a/sdk/test/opentelemetry/sdk/trace/span_test.rb +++ b/sdk/test/opentelemetry/sdk/trace/span_test.rb @@ -324,34 +324,34 @@ describe '#status=' do it 'sets the status' do - span.status = Status.new(Status::ERROR, description: 'cancelled') + span.status = Status.error('cancelled') _(span.status.description).must_equal('cancelled') _(span.status.code).must_equal(Status::ERROR) end it 'does not set the status if span is ended' do span.finish - span.status = Status.new(Status::ERROR, description: 'cancelled') + span.status = Status.error('cancelled') _(span.status.code).must_equal(Status::UNSET) end it 'does not set the status if asked to set to UNSET' do - span.status = Status.new(Status::ERROR, description: 'cancelled') - span.status = Status.new(Status::UNSET, description: 'actually, maybe it is OK?') + span.status = Status.error('cancelled') + span.status = Status.unset('actually, maybe it is OK?') _(span.status.description).must_equal('cancelled') _(span.status.code).must_equal(Status::ERROR) end it 'does not override the status once set to OK' do - span.status = Status.new(Status::OK, description: 'nothing to see here') - span.status = Status.new(Status::ERROR, description: 'cancelled') + span.status = Status.ok('nothing to see here') + span.status = Status.error('cancelled') _(span.status.description).must_equal('nothing to see here') _(span.status.code).must_equal(Status::OK) end it 'allows overriding ERROR with OK' do - span.status = Status.new(Status::ERROR, description: 'cancelled') - span.status = Status.new(Status::OK, description: 'nothing to see here') + span.status = Status.error('cancelled') + span.status = Status.ok('nothing to see here') _(span.status.description).must_equal('nothing to see here') _(span.status.code).must_equal(Status::OK) end From 4971bc3db1864fbbd651a19200ce38cc24fb4988 Mon Sep 17 00:00:00 2001 From: Francis Bogsanyi Date: Fri, 11 Jun 2021 16:18:56 -0400 Subject: [PATCH 4/7] fix test --- .../rack/middlewares/tracer_middleware_test.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/instrumentation/rack/test/opentelemetry/instrumentation/rack/middlewares/tracer_middleware_test.rb b/instrumentation/rack/test/opentelemetry/instrumentation/rack/middlewares/tracer_middleware_test.rb index b18927bba..468ae119b 100644 --- a/instrumentation/rack/test/opentelemetry/instrumentation/rack/middlewares/tracer_middleware_test.rb +++ b/instrumentation/rack/test/opentelemetry/instrumentation/rack/middlewares/tracer_middleware_test.rb @@ -61,12 +61,15 @@ _(first_span.attributes['http.method']).must_equal 'GET' _(first_span.attributes['http.status_code']).must_equal 200 _(first_span.attributes['http.target']).must_equal '/' - _(first_span.status.code).must_equal OpenTelemetry::Trace::Status::OK _(first_span.attributes['http.url']).must_be_nil _(first_span.name).must_equal '/' _(first_span.kind).must_equal :server end + it 'does not explicitly set status OK' do + _(first_span.status.code).must_equal OpenTelemetry::Trace::Status::UNSET + end + it 'has no parent' do _(first_span.parent_span_id).must_equal OpenTelemetry::Trace::INVALID_SPAN_ID end From 64f85309f50c0447b8937ddfd7426d22522fdf0f Mon Sep 17 00:00:00 2001 From: Francis Bogsanyi Date: Fri, 11 Jun 2021 16:26:38 -0400 Subject: [PATCH 5/7] appease the cop --- .../lib/opentelemetry/instrumentation/ethon/patches/easy.rb | 2 +- .../instrumentation/excon/middlewares/tracer_middleware.rb | 2 +- .../instrumentation/faraday/middlewares/tracer_middleware.rb | 2 +- .../lib/opentelemetry/instrumentation/http/patches/client.rb | 2 +- .../instrumentation/http_client/patches/client.rb | 2 +- .../instrumentation/net/http/patches/instrumentation.rb | 2 +- .../instrumentation/rack/middlewares/tracer_middleware.rb | 2 +- .../instrumentation/restclient/patches/request.rb | 4 ++-- .../instrumentation/sinatra/middlewares/tracer_middleware.rb | 2 +- 9 files changed, 10 insertions(+), 10 deletions(-) diff --git a/instrumentation/ethon/lib/opentelemetry/instrumentation/ethon/patches/easy.rb b/instrumentation/ethon/lib/opentelemetry/instrumentation/ethon/patches/easy.rb index dc4172ef2..58d3cbaff 100644 --- a/instrumentation/ethon/lib/opentelemetry/instrumentation/ethon/patches/easy.rb +++ b/instrumentation/ethon/lib/opentelemetry/instrumentation/ethon/patches/easy.rb @@ -42,7 +42,7 @@ def complete # rubocop:disable Metrics/MethodLength @otel_span.status = OpenTelemetry::Trace::Status.error("Request has failed: #{message}") else @otel_span.set_attribute('http.status_code', response_code) - @otel_span.status = OpenTelemetry::Trace::Status.error unless (100..399) === response_code.to_i + @otel_span.status = OpenTelemetry::Trace::Status.error unless (100..399).include?(response_code.to_i) end ensure @otel_span.finish diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index 34894b4f2..b5b67bb09 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb @@ -79,7 +79,7 @@ def handle_response(datum) # rubocop:disable Metrics/AbcSize, Metrics/MethodLeng if datum.key?(:response) response = datum[:response] span.set_attribute('http.status_code', response[:status]) - span.status = OpenTelemetry::Trace::Status.error unless (100..399) === response[:status].to_i + span.status = OpenTelemetry::Trace::Status.error unless (100..399).include?(response[:status].to_i) end if datum.key?(:error) diff --git a/instrumentation/faraday/lib/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware.rb b/instrumentation/faraday/lib/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware.rb index 6033f9aa9..5c9cd419d 100644 --- a/instrumentation/faraday/lib/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware.rb +++ b/instrumentation/faraday/lib/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware.rb @@ -58,7 +58,7 @@ def tracer def trace_response(span, response) span.set_attribute('http.status_code', response.status) - span.status = OpenTelemetry::Trace::Status.error unless (100..399) === response.status.to_i + span.status = OpenTelemetry::Trace::Status.error unless (100..399).include?(response.status.to_i) end end end diff --git a/instrumentation/http/lib/opentelemetry/instrumentation/http/patches/client.rb b/instrumentation/http/lib/opentelemetry/instrumentation/http/patches/client.rb index d99e4aa5f..b23869d04 100644 --- a/instrumentation/http/lib/opentelemetry/instrumentation/http/patches/client.rb +++ b/instrumentation/http/lib/opentelemetry/instrumentation/http/patches/client.rb @@ -38,7 +38,7 @@ def annotate_span_with_response!(span, response) status_code = response.status.to_i span.set_attribute('http.status_code', status_code) - span.status = OpenTelemetry::Trace::Status.error unless (100..399) === status_code.to_i + span.status = OpenTelemetry::Trace::Status.error unless (100..399).include?(status_code.to_i) end def tracer diff --git a/instrumentation/http_client/lib/opentelemetry/instrumentation/http_client/patches/client.rb b/instrumentation/http_client/lib/opentelemetry/instrumentation/http_client/patches/client.rb index 81f5358dc..1684101f4 100644 --- a/instrumentation/http_client/lib/opentelemetry/instrumentation/http_client/patches/client.rb +++ b/instrumentation/http_client/lib/opentelemetry/instrumentation/http_client/patches/client.rb @@ -41,7 +41,7 @@ def annotate_span_with_response!(span, response) status_code = response.status_code.to_i span.set_attribute('http.status_code', status_code) - span.status = OpenTelemetry::Trace::Status.error unless (100..399) === status_code.to_i + span.status = OpenTelemetry::Trace::Status.error unless (100..399).include?(status_code.to_i) end def tracer diff --git a/instrumentation/net_http/lib/opentelemetry/instrumentation/net/http/patches/instrumentation.rb b/instrumentation/net_http/lib/opentelemetry/instrumentation/net/http/patches/instrumentation.rb index 9b46c88ec..7f8de900d 100644 --- a/instrumentation/net_http/lib/opentelemetry/instrumentation/net/http/patches/instrumentation.rb +++ b/instrumentation/net_http/lib/opentelemetry/instrumentation/net/http/patches/instrumentation.rb @@ -64,7 +64,7 @@ def annotate_span_with_response!(span, response) status_code = response.code.to_i span.set_attribute('http.status_code', status_code) - span.status = OpenTelemetry::Trace::Status.error unless (100..399) === status_code.to_i + span.status = OpenTelemetry::Trace::Status.error unless (100..399).include?(status_code.to_i) end def tracer diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/tracer_middleware.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/tracer_middleware.rb index a99fd2db5..748c1dd56 100644 --- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/tracer_middleware.rb +++ b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/tracer_middleware.rb @@ -153,7 +153,7 @@ def create_request_span_name(request_uri_or_path_info, env) end def set_attributes_after_request(span, status, headers, _response) - span.status = OpenTelemetry::Trace::Status.error unless (100..399) === status.to_i + span.status = OpenTelemetry::Trace::Status.error unless (100..399).include?(status.to_i) span.set_attribute('http.status_code', status) # NOTE: if data is available, it would be good to do this: diff --git a/instrumentation/restclient/lib/opentelemetry/instrumentation/restclient/patches/request.rb b/instrumentation/restclient/lib/opentelemetry/instrumentation/restclient/patches/request.rb index bada697fe..ba1e21e1b 100644 --- a/instrumentation/restclient/lib/opentelemetry/instrumentation/restclient/patches/request.rb +++ b/instrumentation/restclient/lib/opentelemetry/instrumentation/restclient/patches/request.rb @@ -43,12 +43,12 @@ def trace_request # rubocop:disable Metrics/AbcSize, Metrics/MethodLength # If so, add additional attributes. if response.is_a?(::RestClient::Response) span.set_attribute('http.status_code', response.code) - span.status = OpenTelemetry::Trace::Status.error unless (100..399) === response.code.to_i + span.status = OpenTelemetry::Trace::Status.error unless (100..399).include?(response.code.to_i) end end rescue ::RestClient::ExceptionWithResponse => e span.set_attribute('http.status_code', e.http_code) - span.status = OpenTelemetry::Trace::Status.error unless (100..399) === e.http_code.to_i + span.status = OpenTelemetry::Trace::Status.error unless (100..399).include?(e.http_code.to_i) raise e ensure diff --git a/instrumentation/sinatra/lib/opentelemetry/instrumentation/sinatra/middlewares/tracer_middleware.rb b/instrumentation/sinatra/lib/opentelemetry/instrumentation/sinatra/middlewares/tracer_middleware.rb index 250e768f7..9c20ffd68 100644 --- a/instrumentation/sinatra/lib/opentelemetry/instrumentation/sinatra/middlewares/tracer_middleware.rb +++ b/instrumentation/sinatra/lib/opentelemetry/instrumentation/sinatra/middlewares/tracer_middleware.rb @@ -43,7 +43,7 @@ def trace_response(span, env, resp) span.set_attribute('http.status_code', status) span.set_attribute('http.route', env['sinatra.route'].split.last) if env['sinatra.route'] span.name = env['sinatra.route'] if env['sinatra.route'] - span.status = OpenTelemetry::Trace::Status.error unless (100..399) === status.to_i + span.status = OpenTelemetry::Trace::Status.error unless (100..399).include?(status.to_i) end end end From 3f7e3f278632961e14fed616f54d5716969bab3b Mon Sep 17 00:00:00 2001 From: Francis Bogsanyi Date: Fri, 11 Jun 2021 17:07:01 -0400 Subject: [PATCH 6/7] fix calls to Status.new in instrumentation --- .../opentelemetry/instrumentation/resque/patches/resque_job.rb | 2 +- .../sidekiq/middlewares/server/tracer_middleware.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation/resque/lib/opentelemetry/instrumentation/resque/patches/resque_job.rb b/instrumentation/resque/lib/opentelemetry/instrumentation/resque/patches/resque_job.rb index 275087ae2..eab7569fe 100644 --- a/instrumentation/resque/lib/opentelemetry/instrumentation/resque/patches/resque_job.rb +++ b/instrumentation/resque/lib/opentelemetry/instrumentation/resque/patches/resque_job.rb @@ -47,7 +47,7 @@ def perform # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Metrics/Cyc super rescue Exception => e # rubocop:disable Lint/RescueException span.record_exception(e) - span.status = OpenTelemetry::Trace::Status.new(OpenTelemetry::Trace::Status::ERROR, description: "Unhandled exception of type: #{e.class}") + span.status = OpenTelemetry::Trace::Status.error("Unhandled exception of type: #{e.class}") raise e ensure span.finish diff --git a/instrumentation/sidekiq/lib/opentelemetry/instrumentation/sidekiq/middlewares/server/tracer_middleware.rb b/instrumentation/sidekiq/lib/opentelemetry/instrumentation/sidekiq/middlewares/server/tracer_middleware.rb index b36c926df..c79777313 100644 --- a/instrumentation/sidekiq/lib/opentelemetry/instrumentation/sidekiq/middlewares/server/tracer_middleware.rb +++ b/instrumentation/sidekiq/lib/opentelemetry/instrumentation/sidekiq/middlewares/server/tracer_middleware.rb @@ -46,7 +46,7 @@ def call(_worker, msg, _queue) # rubocop:disable Metrics/AbcSize, Metrics/Cyclom yield rescue Exception => e # rubocop:disable Lint/RescueException span.record_exception(e) - span.status = OpenTelemetry::Trace::Status.new(OpenTelemetry::Trace::Status::ERROR, description: "Unhandled exception of type: #{e.class}") + span.status = OpenTelemetry::Trace::Status.error("Unhandled exception of type: #{e.class}") raise e ensure span.finish From f71d593015b2de008d8d2ba370db126b30a3ddd8 Mon Sep 17 00:00:00 2001 From: Francis Bogsanyi Date: Fri, 11 Jun 2021 17:29:35 -0400 Subject: [PATCH 7/7] appease the cop --- .../lib/opentelemetry/instrumentation/ethon/patches/easy.rb | 2 +- .../instrumentation/excon/middlewares/tracer_middleware.rb | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/instrumentation/ethon/lib/opentelemetry/instrumentation/ethon/patches/easy.rb b/instrumentation/ethon/lib/opentelemetry/instrumentation/ethon/patches/easy.rb index 58d3cbaff..c8af24f06 100644 --- a/instrumentation/ethon/lib/opentelemetry/instrumentation/ethon/patches/easy.rb +++ b/instrumentation/ethon/lib/opentelemetry/instrumentation/ethon/patches/easy.rb @@ -32,7 +32,7 @@ def perform super end - def complete # rubocop:disable Metrics/MethodLength + def complete begin response_options = mirror.options response_code = (response_options[:response_code] || response_options[:code]).to_i diff --git a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb index b5b67bb09..1f2c7ee94 100644 --- a/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb +++ b/instrumentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb @@ -71,7 +71,7 @@ def self.around_default_stack private - def handle_response(datum) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength + def handle_response(datum) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity: if datum.key?(:otel_span) datum[:otel_span].tap do |span| return span if span.end_timestamp @@ -82,9 +82,7 @@ def handle_response(datum) # rubocop:disable Metrics/AbcSize, Metrics/MethodLeng span.status = OpenTelemetry::Trace::Status.error unless (100..399).include?(response[:status].to_i) end - if datum.key?(:error) - span.status = OpenTelemetry::Trace::Status.error("Request has failed: #{datum[:error]}") - end + span.status = OpenTelemetry::Trace::Status.error("Request has failed: #{datum[:error]}") if datum.key?(:error) span.finish datum.delete(:otel_span)