Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renaming Meter to Accumulator in Metrics SDK context #6

Closed
wants to merge 2 commits into from

Conversation

AzfaarQureshi
Copy link

@AzfaarQureshi AzfaarQureshi commented Nov 9, 2020

Description

This PR addresses Issue open-telemetry#1342 and issue open-telemetry#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
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
  1. 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

@AzfaarQureshi AzfaarQureshi marked this pull request as ready for review November 9, 2020 23:36
@AzfaarQureshi AzfaarQureshi self-assigned this Nov 9, 2020
@AzfaarQureshi AzfaarQureshi force-pushed the 1342-rename-meter-to-accumulator branch from e5f8050 to d04f0f9 Compare November 9, 2020 23:40
@AzfaarQureshi AzfaarQureshi force-pushed the 1342-rename-meter-to-accumulator branch from d04f0f9 to baa6b10 Compare November 9, 2020 23:54
@AzfaarQureshi AzfaarQureshi changed the title renaming meter to accumulator in metrics sdk context renaming Meter to Accumulator in metrics sdk context Nov 9, 2020
@AzfaarQureshi AzfaarQureshi force-pushed the 1342-rename-meter-to-accumulator branch 8 times, most recently from 17d5feb to 335e3b0 Compare November 10, 2020 01:40
@AzfaarQureshi AzfaarQureshi force-pushed the 1342-rename-meter-to-accumulator branch from 335e3b0 to 2edb265 Compare November 10, 2020 01:48
@alolita alolita changed the title renaming Meter to Accumulator in metrics sdk context Renaming Meter to Accumulator in Metrics SDK context Nov 10, 2020
Copy link

@alolita alolita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants