Renaming Meter to Accumulator in Metrics SDK context

[AzfaarQureshi]

Description

This PR addresses Issue #1342 and issue #1307 by renaming the Meter class in the metrics sdk to Accumulator

Summary of changes

1. rename Meter class to Accumulator
2. rename all meter = meter_provider.get_meter() to accumulator = meter_provider.get_meter() for consistency

Rationale behind changes

1. rename Meter class to Accumulator

• The Meter Class does a lot of the same things as the Accumulator in Go

Similarities	Meter class	Accumulator struct
collects all metrics	def collect	func collect
helper func to collect from sync instruments	def _collect_metrics	func collectSyncInstruments
helper func to collect from async instruments	def _collect_observers	func observeAsyncInstruments
record batch metric events	def record_batch	func RecordBatch
register sync instruments	create_counter, create_updowncounter etc	func NewSyncInstrument
register async instruments	register_sumobserver, register_updownsumobserver etc	func NewAsyncInstrument

• The Meter class does all of what is defined for theAccumulator in the spec
  • extends the Meter interface
  • adds collection methods for Synchronous and Asynchronous instruments (which are metrics and observers respectively in Python)
  • registers records to concurrency update and aggregate records of metric data

2. rename all meter = meter_provider.get_meter() to accumulator = meter_provider.get_meter() for consistency

• feels awkward to have an instance of a meter since a meter is an interface
• now that the metrics sdk meter_provider returns an accumulator, the usage in various tests and examples should reflect what is actually being returned

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Just made sure all the tests are passing post name changes.

Checklist:

• Followed the style guidelines of this project
• Changelogs have been updated
• Unit tests have been added
• Documentation has been updated

cc - @ocelotl

[lzchen]

Getting an accumulator from get_meter seems so weird. As a user, I wouldn't want to be bombarded by more terms. I'm wondering why we the specs needs to introduce a new word to represent the instrumentation. We don't have this for Tracer.

[AzfaarQureshi]

As a user, I wouldn't want to be bombarded by more terms.

yup, you do raise a good point. But IMO if we're renaming the Meter class to Accumulator to comply with the spec, I think we also need to update its usages as well. Otherwise it'll be even more confusing because meter is an interface and meter = metrics.get_meter() gives the impression that you're directly instantiating an object of the interface.

Getting an accumulator from get_meter seems so weird

lol you're right, I wonder if we can fix this incongruity between method name and return type 🤔

This kind of makes sense to me because meter is the super type of accumulator so you're still getting a meter from the method. That said, I can also see how get_meter() makes it sound like you're explicity expecting a meter object.

The Go repo actually renamed get_meter() to meter() CHANGELOG. Do you think it would help add clarity to make the same change here? This way, with metrics.meter(), you're just accessing whichever meter-type object the meterprovider is exposing (which could be anything since meter is just an interface)

Do you have another approach in mind?

[lzchen]

The Go repo actually renamed get_meter() to meter()

I don't believe we should be following the Go implementation as the source of truth, rather the api specs say GetMeter. I don't know if this is outdated but we can change it if it changes in the future.

I think we also need to update its usages as well.

With that being said, I don't think the usage documents should be renamed. Accumulator is an implemenation in the sdk of the meter interface defined in the api, so it is purely an sdk concept. The usage documents don't guarantee the usage of the sdk (even though we are instantiating the MeterProvider from the SDK`). Looking at these examples, if I were a user coming in with my own sdk implementation of the meter, I don't know what the accumulator is. Also take a look at some of the instrumentations that use the meter but only depend on the api. It doesn't make sense for them to refer it as accumulators, so why should we even introduce that term to users? Meter is the only word that all users would be familiar with, whether or not they are using the sdk.

I think this whole naming thing is weird in the specs itself, but it's fine right now to adhere to it. Just don't be surprised if there are changes in the future :D

[AzfaarQureshi]

Alright makes sense, i'll revert the usage change

[aabmass]

My understanding is that Accumulator doesn't have to be the SDK meter, but that is an implementation detail of the Python and Go SDKs (see this comment thread open-telemetry/opentelemetry-specification#880 (comment))

I think it's even still a bit confusing if type(metrics.get_meter()) is Accumulator instead of Meter

[toumorokoshi]

A best of both worlds approach (and this may require spec changes) may be to add a new Meter object, whose only job is to create instruments that bind to the accumulator.

IMO this is actually a fairly clean break: Meter owns the logic required create new instruments, and accumulation can be the first part of the processing pipeline.

[ocelotl]

I understand the valid concerns raised in the conversation. Nevertheless, I think the confusion related to the names Meter or Accumulator should be resolved in the specification if they are not solved there yet. We do the right thing by following the specification as best as possible and by trying to also be similar to other implementations where possible. I think this is a step in that direction, approving.

[toumorokoshi]

I agree with the whole discussion above, but also with @ocelotl: this is what the spec says for now, so adhering to it and raising discussions is the best choice.

[toumorokoshi]

please update the description. I believe it's incorrect that any meter from any SDK will work in this architecture.

[AzfaarQureshi]

Is this more accurate?
accumulator: The meter from the Metrics API used to collect metrics.

[AzfaarQureshi]

rebased now that #1373 got merged

[NathanielRN]

@AzfaarQureshi @lzchen It looks like this PR broke the Contrib Tests 😞 .

This was caught in the PR to add Contrib Tests to Core repos but it hasn't been merged yet #1357

We'll have to make an equivalent PR in the Contrib Repo to fix these references:

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/c8904cef1a75f44618f12164962cc64168d472d7/instrumentation/opentelemetry-instrumentation-grpc/src/opentelemetry/instrumentation/grpc/_client.py#L93-L95

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/c8904cef1a75f44618f12164962cc64168d472d7/instrumentation/opentelemetry-instrumentation-system-metrics/src/opentelemetry/instrumentation/system_metrics/__init__.py#L96-L98

And bump the Contrib Repo CORE_REPO_SHA to this the point where this PR was merged

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/master/.github/workflows/test.yml#L9

[AzfaarQureshi]

@NathanielRN o rip mb, i'll put up an equivalent PR in contrib tonight! 😄