fix!: total order constraint on span.status= (#805)

* fix: total order constraint on span.status= * fix!: remove http_to_status helper * fix tests & other uses of Status.new * fix test * appease the cop * fix calls to Status.new in instrumentation * appease the cop
open-telemetry · Jun 14, 2021 · 55b6be1 · 55b6be1
1 parent 2401964
commit 55b6be1
Show file tree

Hide file tree

Showing 23 changed files with 100 additions and 146 deletions.
diff --git 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/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/lib/opentelemetry/trace/util/http_to_status.rb b/api/lib/opentelemetry/trace/util/http_to_status.rb
diff --git a/api/test/opentelemetry/trace/status_test.rb b/api/test/opentelemetry/trace/status_test.rb
@@ -9,82 +9,34 @@
 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)
+      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
@@ -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
@@ -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
@@ -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/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
@@ -32,22 +32,17 @@ 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
               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).include?(response_code.to_i)
               end
             ensure
               @otel_span.finish

diff --git a/...umentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb b/...umentation/excon/lib/opentelemetry/instrumentation/excon/middlewares/tracer_middleware.rb
@@ -71,25 +71,18 @@ 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
 
                 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).include?(response[:status].to_i)
                 end
 
-                if datum.key?(:error)
-                  span.status = OpenTelemetry::Trace::Status.new(
-                    OpenTelemetry::Trace::Status::ERROR,
-                    description: "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)

diff --git a/...tation/faraday/lib/opentelemetry/instrumentation/faraday/middlewares/tracer_middleware.rb b/...tation/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).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
@@ -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).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
@@ -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).include?(status_code.to_i)
           end
 
           def tracer

diff --git a/...umentation/net_http/lib/opentelemetry/instrumentation/net/http/patches/instrumentation.rb b/...umentation/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).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
@@ -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).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/...tation/rack/test/opentelemetry/instrumentation/rack/middlewares/tracer_middleware_test.rb b/...tation/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

diff --git 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/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