Rename Record class in Metrics SDK to Accumulation to follow spec

[AzfaarQureshi]

Description

This PR addresses Issue #1343 and issue #1307 by renaming the Record class in the metrics sdk to Accumulation

related to PR #1372 and issue #1342

Summary of Changes

1. rename Record class to Accumulation
2. rename all record = metrics.Accumulation() to accumulation = metrics.Accumulation() for consistency

Rationale behind changes

1. rename Record class to Accumulation

• PR Renaming Meter to Accumulator in Metrics SDK context #1372 showed that the Meter class should be renamed to Accumulator
• According to the Spec, the type computed/collected by the Accumulator must be named Accumulation

2. rename all record = metrics.Accumulation() to accumulation = metrics.Accumulation() for consistency

• cleaning up vestigial naming scheme from when Accumulation used to be called Record.
• variable name should reflect what is actually behind 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 naming change

Checklist:

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

cc - @ocelotl

[lzchen]

CHANGELOG?

[lzchen]

@ocelotl
Taking a look at the specs, and Accumulation is defined as consists of Instrument, Label Set, Resource, and Aggregator snapshot, output by Accumulator. So Record is the "output" of the accumulator, however it doesn't contain the Resource. I think the specs is actually incorrect in specifying that Resource should be part of Record, as the resource value is always the same. Maybe open up an issue in the specs to change this wording?

[toumorokoshi]

@lzchen random question: it looks like there's also a export.MetricRecord that is almost identical (it includes an additional resource). Do we need that duplication?

[toumorokoshi]

Ah, I suppose the spec calls for it... although ExportRecord should include an additional timestamp field.

[toumorokoshi]

I think the specs is actually incorrect in specifying that Resource should be part of Record, as the resource value is always the same. Maybe open up an issue in the specs to change this wording?

I think this is intentional, there's strong wording that this needs to be possible:

The Accumulator MUST provide the option to associate a Resource with the Accumulations that it produces.

The other option would be to move that association in the processor, which comes with the caveat that all accumulations must be associated with the same resource.

If I had to guess, the reason why it's designed this way is that processors / exporters might need to consume accumulations from multiple meters, so knowing what resource to use is helpful.

[toumorokoshi]

This change make sense to me! Would be good to see the other desired changes (e.g. declaration of an accumulator class) in future PRs.

[lzchen]

@toumorokoshi

The other option would be to move that association in the processor, which comes with the caveat that all accumulations must be associated with the same resource.

Right this was my thinking as well. If the a Resource must be part of the result from an accumulator, then we have to change Accumulation to include the resource field as well. With this change, resource should be removed from the processor. ExportRecord can be changed in a future pr to accept a timestamp.

[AzfaarQureshi]

@lzchen Added changes to the changelog!

This change make sense to me! Would be good to see the other desired changes (e.g. declaration of an accumulator class) in future PRs.

@toumorokoshi yup a PR for that is up too #1372 would appreciate a review 😄

[ocelotl]

Approving, even if there is still confusion regarding the nomenclature of the components (understandable, the specification is still evolving) it is the right thing to do to be compliant even if changes are quite likely in the foreseeable future.

[lzchen]

@AzfaarQureshi
Can you pull from the latest master and then I can merge? For some reason github isn't letting me.

[AzfaarQureshi]

@lzchen rebased!

[lzchen]

@AzfaarQureshi
Sorry it's still out of date. Once more?

[AzfaarQureshi]

@lzchen rebased again

[codeboten]

Looks like another update to the branch is needed

[AzfaarQureshi]

lol, @codeboten rebased again :)