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

Refactor Python Metrics SDK to use naming consistent with Metrics Specification #1307

Closed
shovnik opened this issue Oct 30, 2020 · 5 comments
Closed
Labels
1.10.0rc1 release candidate 1 for metrics GA backlog metrics release:required-for-ga To be resolved before GA release sdk Affects the SDK package.

Comments

@shovnik
Copy link
Contributor

shovnik commented Oct 30, 2020

While preparing to work on a new exporter for the Python SDK I noticed some differences in naming between the Python implementation and the Metrics Spec. Most of the spec non-compliance can be fixed with renaming. Since the Metrics SDK spec was based off the Go SDK, I compared the Go implementation with the Python to see what changes needed to be made.

I’d like to discuss renaming some of these classes, methods and files to comply with the Metrics SDK Spec. In addition, the new naming will facilitate comparing the current state of the Python SDK with the metric compliance matrix in the future.

Current Understanding of Metrics Export Pipeline Implementation

Spec Item Notes Location
Accumulator Extends Meter interface by adding collection methods for both metrics (Synchronous Instruments) and observers (Asynchronous Instruments). Registers Recorders which concurrently updates and aggregator with records of metric data. These records are gathered into Accumulations produced by the aggregators. sdk/metrics/init.py
Controller Coordinates data flow within export pipeline by collecting one Accumulation, processing it into an ExportRecordSet and exporting it once every fixed interval. sdk/metrics/export/controller.py
Processor Transforms Accumulation into ExportRecordSet applying different transformations such reducing dimensionality, or conversion between Delta/Cumulative representation depending on custom processor with the process() method. sdk/metrics/export/processor.py
Exporter Custom component that converts ExportRecordSet data into correct format before sending to target backend. exporter/*
Aggregator Holds aggregate values and takes snapshot every export interval. sdk/metrics/export/aggregate.py

Naming Change Suggestions

Source File Current Name New Name Reference/Rationale
sdk/metrics/init.py Meter Accumulator Meter class in Python does the same things as the Accumulator in Go and performs the same functionality was what is defined in the spec for the Accumulator.
sdk/metrics/init.py Record Accumulation Name of type outputted from the Accumulator and inputted into the Processor should be Accumulation (spec).
sdk/metrics/view.py View Record Equivalency established from the Go-SDK (sdk.go). Unsure about this change due to Views API coming from OpenCensus and being a WIP.
sdk/metrics/view.py ViewData RecordData To be consistent with changing the View name to Record
sdk/metrics/view.py ViewManager RecordManager To be consistent with changing the View name to Record
sdk/metrics/view.py ViewConfig RecordConfig To be consistent with changing the View name to Record
sdk/metrics/view.py get_view_data get_record To be consistent with changing the View name to Record
sdk/metrics/export/init.py MetricRecord ExportRecord Name of Type transferred from Processor to Exporter (spec).
sdk/metrics/export/init.py MetricsExporter Exporter Main metric exporter class named Exporter in both spec and metric.go.
sdk/metrics/export/processor.py finished_collection finish_collection Unrelated to spec, method just does not return anything so "finish" would be more appropriate than "finished".
sdk/metrics/export/processor.py metric_records export_record_set Name of Type transferred from Processor to Exporter (spec).
sdk/metrics/export/aggregate.py aggregate.py aggregator.py Consistent with class name: Aggregator. All the other component file names are based on their class name. (aggregator.go)
sdk/metrics/init.py Metric SyncIntrument Metrics spec refers to the python SDK concept of metric as synchronous instrument.
sdk/metrics/init.py Observer AsyncInstrument Metrics spec refers to the python SDK concept of observer as asynchronous instrument.

Expected Impact

If approved, any renaming should also be done in the related tests and documentation. No repercussions are expected since the metric SDK is not generally available.

Additional question: should the entire view.py file containing the ViewManager logic could be moved into the init.py file containing the Meter(Accumulator) class since its sole purpose is to support it.

cc - @alolita, @AzfaarQureshi

@lzchen lzchen added metrics sdk Affects the SDK package. labels Oct 30, 2020
@lzchen
Copy link
Contributor

lzchen commented Oct 31, 2020

@ocelotl fyi.
Might be good to add to your list of metric issues.

@shovnik
Copy link
Contributor Author

shovnik commented Nov 3, 2020

Another difference I noticed is that the Python metric spec calls synchronous and asynchronous instruments (described in spec and Go implementation) as metrics and observers. This seems intentional, but it might be worth renaming as well to be aligned with spec.

@ocelotl
Copy link
Contributor

ocelotl commented Nov 4, 2020

Thanks, I'll review this and update the issues I opened yesterday.

@github-actions
Copy link

github-actions bot commented Apr 9, 2021

This issue was marked stale due to lack of activity. It will be closed in 30 days.

@lzchen
Copy link
Contributor

lzchen commented Apr 19, 2021

Closing due to major reworking in the metrics api/sdk

@lzchen lzchen closed this as completed Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.10.0rc1 release candidate 1 for metrics GA backlog metrics release:required-for-ga To be resolved before GA release sdk Affects the SDK package.
Projects
None yet
Development

No branches or pull requests

4 participants