Add _ViewInstrumentMatch by ocelotl · Pull Request #2400 · open-telemetry/opentelemetry-python

ocelotl · 2022-01-22T00:28:57Z

This class is intentionally named ViewInstrumentMatch because it represents a match between a View and an Instrument. There will be as many instances of ViewInstrumentMatch as there will be matches between Views and Instruments. I think this is a more descriptive name than ViewStorage considering the fact that instances of this class do no store any Views.

aabmass · 2022-01-24T20:14:21Z

because it represents a match between a View and an Instrument.

I think this is a more descriptive name than ViewStorage considering the fact that instances of this class do no store any Views.

It doesn't store views, it is the storage for a view. To me, ViewStorage is more descriptive of what the class does and ViewInstrumentMatch is just descriptive of how it gets created. Also, we have MetricReaderStorage so "storage" is consistent.

Also curious what folks besides me and Diego think–here's the section of the design doc for this

codeboten · 2022-01-28T23:19:44Z

For what it's worth, I think other implementations are going with ViewsRegistry here (at least java/js are).

codeboten

Sorry I believe RegisteredView is the how the component which ties a view to instruments (or an instrument selector) is described in JS and in Java:

https://github.com/open-telemetry/opentelemetry-java/blob/0e38773e4530a0264a0bccfd3cb8629cd0c8094f/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/RegisteredView.java

https://github.com/open-telemetry/opentelemetry-js/blob/9b5feb2f21f43758fd9b4e711319739db4d63ec0/experimental/packages/opentelemetry-sdk-metrics-base/src/view/ViewRegistry.ts

@lzchen @owais @lonewolf3739 thoughts on what name is best here?

codeboten · 2022-02-11T23:36:11Z

It doesn't store views, it is the storage for a view

But it's also not the storage for a view, as a single view may be associated with many of these objects (one per instrument) correct? In which case the relationship between ViewInstrument described in ViewInstrumentMatch makes more sense to me.

I don't know that Match is descriptive enough to make it clear as to what this thing is though, I don't know what I'm supposed to do with a Match.

Fixes open-telemetry#2300

opentelemetry-sdk/src/opentelemetry/sdk/_metrics/_view_instrument_match.py

opentelemetry-sdk/tests/metrics/test_view_instrument_match.py

ocelotl · 2022-02-12T00:01:37Z

I don't know that Match is descriptive enough to make it clear as to what this thing is though, I don't know what I'm supposed to do with a Match.

Actually, the user does not do anything with a view and instrument match. They are an implementation detail, I have updated the class name and the PR title to reflect this. Implementation details should still have descriptive names.

It is hard to describe what this class does, it has a consume_measurement and a collect method as MetricReaderStorage and MeasurementConsumer do. Instances of this class will be used like this.

A measurement that follows the consume_measurement process is handled by the following methods:

Instrument.add/record
MeasurementConsumer.consume_measurement
MetricReader.consume_measurement
MetricReaderStorage.consume_measurement
ViewInstrumentMatch.consume_measurement
...

From point 1 to point 4 the measurement is handled sequentially by only one object. In step 5 the measurement is handled by several objects (instances of ViewInstrumentMatch) because it can go in several metric streams, one per match between a view and an instrument.

Because it is hard to describe what this class does or is, I believe it is better to stick to the facts, view and instruments can match and every time they match we get an instance of this class. It would be perfect if we could find a noun or adjective that describes it exactly, but I don't know one and I don't think either storage or registry describe them either.

codeboten

ViewInstrumentJoin? ViewInstrumentUnion? I'm just throwing more ideas to see if any stick 😄

I'm ok with calling this a Match as well, tbh since this is an implementation detail I don't have a strong preference here.

lzchen · 2022-02-14T18:48:40Z

implementation detail I don't have a strong preference here.

Same here. _ViewInstrumentMatch is fine with m.

aabmass · 2022-02-15T19:05:17Z

I agree with @codeboten's point about "match"

I don't know that Match is descriptive enough to make it clear as to what this thing is though, I don't know what I'm supposed to do with a Match.

but i'm fine to move on with the this name 👍

aabmass · 2022-02-17T19:26:34Z

opentelemetry-sdk/src/opentelemetry/sdk/_metrics/_view_instrument_match.py

+        name: str,
+        unit: str,
+        description: str,
+        aggregation: type,


What does type mean here?

aabmass · 2022-02-17T19:28:26Z

opentelemetry-sdk/src/opentelemetry/sdk/_metrics/_view_instrument_match.py

+class _ViewInstrumentMatch:
+    def __init__(
+        self,
+        name: str,


Why not just pass in the view here to get all of these properties? They are well defined there and then if more fields are added to the view, you don't need to update this constructor

These attributes don't necessarily come from the view, they may come from the instrument, like this.

aabmass · 2022-02-17T19:29:35Z

opentelemetry-sdk/src/opentelemetry/sdk/_metrics/_view_instrument_match.py

+                if key in self._attribute_keys:
+                    attributes[key] = value
+        else:
+            attributes = measurement.attributes


this can be None

Right, @codeboten fixed this in #2469. ✌️

aabmass · 2022-02-17T19:33:02Z

opentelemetry-sdk/src/opentelemetry/sdk/_metrics/_view_instrument_match.py

+
+        attributes = frozenset(attributes.items())
+
+        if attributes not in self._attributes_aggregation.keys():


.keys() is unnecessary here

opentelemetry-sdk/src/opentelemetry/sdk/_metrics/_view_instrument_match.py

ocelotl added metrics Skip Changelog PRs that do not require a CHANGELOG.md entry labels Jan 22, 2022

ocelotl self-assigned this Jan 22, 2022

ocelotl requested a review from a team January 22, 2022 00:28

ocelotl added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Jan 24, 2022

ocelotl mentioned this pull request Jan 24, 2022

Add _ViewInstrumentMatch #2301

Closed

codeboten reviewed Feb 11, 2022

View reviewed changes

Add _ViewInstrumentMatch

6c4949c

Fixes open-telemetry#2300

ocelotl force-pushed the issue_2300 branch from d6ebc41 to 6c4949c Compare February 11, 2022 23:42

ocelotl changed the title ~~Add ViewInstrumentMatch~~ Add _ViewInstrumentMatch Feb 11, 2022

codeboten reviewed Feb 11, 2022

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/_metrics/_view_instrument_match.py Outdated Show resolved Hide resolved

opentelemetry-sdk/tests/metrics/test_view_instrument_match.py Outdated Show resolved Hide resolved

codeboten reviewed Feb 12, 2022

View reviewed changes

WIP

05222d1

Fix issues

c39aaa2

ocelotl requested a review from codeboten February 15, 2022 22:40

codeboten approved these changes Feb 15, 2022

View reviewed changes

lzchen approved these changes Feb 17, 2022

View reviewed changes

Allow empty frozensets for attributes

6635294

lzchen merged commit a07e039 into open-telemetry:main Feb 17, 2022

ocelotl deleted the issue_2300 branch February 17, 2022 18:36

aabmass reviewed Feb 17, 2022

View reviewed changes

This was referenced Feb 17, 2022

Add check for race condition #2470

Closed

Add type annontations #2471

Closed


		attributes = frozenset(attributes.items())

		if attributes not in self._attributes_aggregation.keys():

Conversation

ocelotl commented Jan 22, 2022

Uh oh!

aabmass commented Jan 24, 2022

Uh oh!

codeboten commented Jan 28, 2022

Uh oh!

codeboten left a comment

Choose a reason for hiding this comment

Uh oh!

codeboten commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ocelotl commented Feb 12, 2022

Uh oh!

codeboten left a comment

Choose a reason for hiding this comment

Uh oh!

lzchen commented Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabmass commented Feb 15, 2022

Uh oh!

aabmass Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

aabmass Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

ocelotl Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

aabmass Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

ocelotl Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

aabmass Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codeboten commented Feb 11, 2022 •

edited

Loading

lzchen commented Feb 14, 2022 •

edited

Loading