Implement optional strict mode

[elskwid]

Overview

We recently removed runtime checks from the API and the SDK. During the SIG meeting on October 16, 2019, we discussed the value of having a strict mode for the library. We believe there is value in having an optional strict mode where a runtime flag can be enabled that raises helpful errors for the developer/environment.

Rationale

The error handling spec states that implementations MUST NOT throw unhandled exceptions at run time with more to say regarding performance and options. Additionally, the spec mentions that an implementation can implement a strict mode.

What needs doing

• Create a flag that enables strict mode for the library
• Add Strict modules inline with classes that have optional strict checks
  • This keeps the modified methods close to the original implementation
  • Eases the burden for contributors and maintenance
• Add tests for all class/modules that have strict checks
• Create a mechanism that will prepend (probably) the Strict modules to the classes when the flag is enabled

Examples / Prior Art

Global flag

OJ has a default_options configuration settiong which allows the user to set the mode. As we don't have multiple modes perhaps this is overkill.

OpenTelemetry.mode = :strict

or

OpenTelemetry.strict = true

Strict modules

The current idea is to keep the strict definitions close to the API/SDK classes and methods. Moving the Strict module into the class that requires the optional checks means it's easier to discover and maintain.

module OpenTelemetry
  module Trace
    class Event
      module Strict
        def initialize(name:, attributes: nil, timestap: nil)
          raise ArgumentError unless name.is_a?(String)
          raise ArgumentError unless StrictHelpers.valid_attributes?(attributes)
        end
      end

      def initialize(name:, attributes: nil, timestamp: nil)
        # default implementation
      end
    end
  end
end

Testing

describe OpenTelemetry::Trace::Event do
  Event = OpenTelemetry::Trace::Event

  describe "Strict mode" do
    StrictEvent = Class.new(Event).prepend(Event::Strict)

    # test strict methods
  end

  # test non-strict/default implementation
end

Wiring it up

Provide a global method that knows what classes have Strict modules and can prepend those modules. The module definition may need to change so we can register what modules need to be enabled/wired up. Note: this is very much a WIP.

module OpenTelemetry
  class Foo
    include OpenTelemetry::Strict

    module Strict
      def bar(s)
        raise ArgumentError unless s.is_a?(String)
        super
      end
    end

    def bar(s)
      puts s
    end
  end
end

And somewhere else:

module OpenTelemetry
  module StrictMode
    def self.enable_all
      self.registered.each { |reg| reg.prepend(reg::Strict) }
    end
  end
end

[elskwid]

I'm working on this now! ⛑

[elskwid]

@mwear here's the write-up for the strict mode as we discussed today.

[benedictfischer09]

Is the amount of overhead required to maintain strict mode too much to make this worthwhile right now? The user facing api is fairly substantial and seems to be changing a lot, if the spec was very stable I think it would make more sense to think about doing this, but I would vote for waiting till at least a 1.0 release

[github-actions]

👋 This issue has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this issue will be closed eventually by the stale bot.