test: Add tests for MeterProvider API

[elias19r]

• Make name a required argument
• Add test for MeterProvider#meter
• Add test for OpenTelemetry.{meter_provider,meter_provider=}

Also,

• Fix error regarding mocking keyword args with minitest's expect method
• Make version a keyword arg

[elias19r]

I noticed that in Metrics API we have meter(name = nil, version = nil) but we have meter(name, version = nil) in Metrics SDK

Specification doesn't say name must be provided, however name is one of the identifying fields and all the other args are optional. Because of that, I wonder if we should make at least name required here in the API: https://opentelemetry.io/docs/reference/specification/metrics/api/#get-a-meter

Looking at JavaScript implementation as an example, name is required in the API: https://github.com/open-telemetry/opentelemetry-js/blob/cd0d8806e0ed4c9dc68be9773770349c712e5e76/api/src/metrics/MeterProvider.ts

Let me know what you think!

[elias19r]

Also, we could take the opportunity to turn version into a keyword arg if that's the pattern we would like to have like suggested here: #1237 (comment)

  def meter(name, version: nil)

And in the future,

  def meter(name, version: nil, schema_url: nil, attributes: {})

[fbogsany]

I think named arguments makes sense. There's some inconsistency between this and the Tracing API because the Metrics API explicitly requires a name whereas the Tracing API waffled a little on whether it was required, whether it was honoured (an implementation can ignore it), and what to do when an invalid value was passed. IMO we should move Metrics in our preferred direction and subsequently update the Tracing interface to match.

[elias19r]

Updated in 1c54ec3

[elias19r]

I'd like to exercise that OpenTelemetry.meter_provider initially returns a ProxyMeterProvider. This test kind of exercises that, but it is not ideal. If this test runs first, then it exercises that; otherwise, it doesn't: it just exercises the reset

I was thinking if a proper reset would be calling load 'opentelemetry-metrics-api.rb to reload the file instead of manually setting @meter_provider with instance_variable_set like OpenTelemetry::TestHelpers suggests

[robertlaurin]

Yeah it's a bit tricky if you're looking to test the initial state before any tests run. I didn't put too much importance on this because in reality all you're testing with that is that this line of code contains no syntax errors.

If this test runs first, then it exercises that; otherwise, it doesn't: it just exercises the reset

If there was an issue in this code path, it would show up as a flakey test, which is not nothing.

[robertlaurin]

Hey @elias19r, sorry for the delay on reviewing this. I'll carve out some time for it tomorrow. Thanks for the PR, excited to see some outside contributions to the metrics endeavour!

[robertlaurin]

My update branch button is missing, think we need to rebase here

Run cd "./semantic_conventions"
Running RuboCop...
RuboCop failed!
Inspecting 8 files
.......C

Offenses:

opentelemetry-semantic_conventions.gemspec:30:3: C: Gemspec/DevelopmentDependencies: Specify development dependencies in Gemfile.
  spec.add_development_dependency 'bundler', '>= 1.17'
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
opentelemetry-semantic_conventions.gemspec:31:3: C: Gemspec/DevelopmentDependencies: Specify development dependencies in Gemfile.
  spec.add_development_dependency 'minitest', '~> 5.0'
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
opentelemetry-semantic_conventions.gemspec:32:3: C: Gemspec/DevelopmentDependencies: Specify development dependencies in Gemfile.
  spec.add_development_dependency 'rake', '~> 12.0'
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
opentelemetry-semantic_conventions.gemspec:33:3: C: Gemspec/DevelopmentDependencies: Specify development dependencies in Gemfile.
  spec.add_development_dependency 'rubocop', '~> 1.3'
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
opentelemetry-semantic_conventions.gemspec:34:3: C: Gemspec/DevelopmentDependencies: Specify development dependencies in Gemfile.
  spec.add_development_dependency 'simplecov', '~> 0.17'
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
opentelemetry-semantic_conventions.gemspec:35:3: C: Gemspec/DevelopmentDependencies: Specify development dependencies in Gemfile.
  spec.add_development_dependency 'yard', '~> 0.9'
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
opentelemetry-semantic_conventions.gemspec:36:3: C: Gemspec/DevelopmentDependencies: Specify development dependencies in Gemfile.
  spec.add_development_dependency 'yard-doctest', '~> 0.1.6'
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^