Add more details to SDK MeterProvider and View by reyang · Pull Request #1730 · open-telemetry/opentelemetry-specification

reyang · 2021-05-28T20:54:47Z

Follow up #1673
Fixes #466
Related issues #1116

Changes

Added details to the MeterProvider SDK spec

jsuereth

My concern here is the following:

View is defined in the SDK as it's mostly a processing piepline thing, however it's really feeling like an APi thing
I think there's a bifurcation of API that needs to happen as we specify between "How a user specified a view" and "The interface and processing of that view". I'm not sure I can tell where the line between the two is in this specification.
Fundamentally, with Views, we're defining a sort of "query" language against metrics (prometheus rewrite rules e.g.). I think we can do something very simple to start with, but I know we'll get pushed this direction. We should set ourselves up for success there. i'd like to see more examples of "configuration" for views.

Do we have a prototype for these that I missed? (sorry the metric API meeting has been at a really inconvenient time for me in general).

jsuereth · 2021-06-01T14:23:40Z

+The MeterProvider MUST provide a way to register Views, and here are the
+inputs:
+
+* The Instrument selection criteria (required), which MUST cover:


From a 'raw level SDK' this seems fine, but as specified i think you NEED a helper method on top of this.

E.g. Can the user just glob-match on Version + Kind to get measurements based on Meter/Instrument-name?

I'm worried the way this language reads right now. I think it makes sense to tease apart "What SDK implementors of metric processors need" and "What SDK Users defining views need". does it make sense to split these two?

Effectively for users, your'e giving them a 'query' language to select metrics.
For the SDK, you're defining a data model "index" that needs to allow rapid processing.

i agree the 4 things you list are what belong on the index.

This is missing something to select "Aggregation". "Customize the aggregation" is a listed use-case.

I don't think this portion of the API needs to specify aggregation. Instead we do need to clarify if any selected instrument has its default aggregation removed, or if it still exists. It's unclear what would happen if I were, e.g. to do a "Select all" here for my metrics. Do they all get removed?

jsuereth · 2021-06-06T18:30:10Z

Based on discussion in the metrics API SiG, updating my comments as follows:

I think it needs to be more explicit when "default hint aggregation" is going to be used or overridden.
- E.g. is the case that always when a selection criteria matches a metric, then the view overrides the default aggregation?
- is there a control to specify "I want the default hint and this view is on top of it?
- How would I know (as a user) that the "hint" API (and default aggregatio) is not used?
i think the selection criteria needs to be specified a little bit more clearly (and ideally with examples).
- Do you allow partial label matching? (Right now it's just instrument
  - can you match just on the existence of Keys (not values)
- Can I regex match? (on anything)
- Were these all intended to be exact-match-only?
- What about "versions" for InstrumentationLibrary/Meter?

jmacd

(partial review, thanks @reyang and sorry for the long delay)

jkwatson · 2021-07-21T17:03:30Z

+  * The `extra dimensions` which come from Baggage/Context (optional). If not
+    provided, no extra dimension will be used. Please note that this only
+    applies to [synchronous Instruments](./api.md#synchronous-instrument).


I wonder if we should leave these out of the first pass. Otherwise, we need to figure out some standard way to figure out how to define the source of the information declaratively.

What do you mean by "define the source of the information"?

I think having this would help us to move forward (once we have the skeleton of the spec, it is easier for others to come and contribute the "meat") 😄

I mean... how do you extract it from the context? Are you providing some sort of function on the context that would give you an attribute?

My understanding is that the SDK via its MeasurementProcessor has access to the context for this purpose.

sure...but how do you define a view to do that work? how do you specify how to extract data from the context?

jsuereth · 2021-07-21T17:07:14Z

@yurishkuro this pr has enough approvals (and discussion during SiG) to merge.

Co-authored-by: John Watson <jkwatson@gmail.com>

jmacd

The recent changes look good and reflect the discussion in the last SIG meeting.

reyang · 2021-07-27T17:01:46Z

Discussed during the Metrics SIG meeting, I've removed the histogram details, we should be ready to merge the PR @jsuereth.

* Add View API * fix link * address comments * provide one example * update wording * polish wording * update wording * mention histogram bucket in the view aggregation * resolve comments based on discussion during the SIG meeting * fix typo * update the spec based on discussion during the SIG meeting * add more diagrams * update the spec based on the discussion during Metrics SIG meeting * minor tweak diagram * address review comment * update the default histogram buckets * remove pipeline from this PR * address comments regarding error handling * Update specification/metrics/sdk.md Co-authored-by: John Watson <jkwatson@gmail.com> * adjust wording * add instrument type and clarify the selection criteria * update the view selection logic * update the view selection logic * fix broken link * update the TODO part by removing the details Co-authored-by: John Watson <jkwatson@gmail.com>

…metry#1730)

reyang requested review from a team May 28, 2021 20:54

github-actions Bot assigned yurishkuro May 28, 2021

reyang added area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory labels May 28, 2021

reyang added this to the Metrics API/SDK Experimental Release milestone May 28, 2021

reyang commented May 28, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md Outdated

reyang commented May 28, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md Outdated

reyang commented May 28, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md

cijothomas reviewed May 28, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md Outdated

cijothomas reviewed May 29, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md

cijothomas reviewed May 29, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md Outdated

jsuereth requested changes Jun 1, 2021

View reviewed changes

alanwest reviewed Jun 3, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md Outdated

victlu reviewed Jun 3, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md Outdated

victlu reviewed Jun 3, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md Outdated

reyang mentioned this pull request Jun 10, 2021

Add the metrics API Hint #1753

Closed

lzchen reviewed Jun 11, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md Outdated

jsuereth reviewed Jun 22, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md Outdated

jmacd reviewed Jun 23, 2021

View reviewed changes

reyang added 3 commits June 23, 2021 22:52

Add View API

4b866a8

fix link

b3259fc

address comments

be9beb5

reyang force-pushed the reyang/metrics-sdk-view branch from 07c6b40 to be9beb5 Compare June 24, 2021 06:36

reyang added 4 commits June 24, 2021 00:49

provide one example

c5c73aa

update wording

f82475e

polish wording

7359e9a

update wording

2df446b

jkwatson reviewed Jul 21, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md Outdated

jkwatson reviewed Jul 21, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md Outdated

jkwatson reviewed Jul 21, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md Outdated

jkwatson reviewed Jul 21, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md Outdated

jkwatson reviewed Jul 21, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md Outdated

jkwatson reviewed Jul 21, 2021

View reviewed changes

Comment thread specification/metrics/sdk.md

Update specification/metrics/sdk.md

2d5a888

Co-authored-by: John Watson <jkwatson@gmail.com>

jmacd mentioned this pull request Jul 22, 2021

Add jaeger remote sampler to SDK configuration #1791

Merged

reyang added 3 commits July 22, 2021 11:20

adjust wording

f1621e7

Merge branch 'main' into reyang/metrics-sdk-view

e606fe5

add instrument type and clarify the selection criteria

4d0913a

yurishkuro removed their assignment Jul 26, 2021

reyang added 4 commits July 26, 2021 10:13

update the view selection logic

6261507

update the view selection logic

954102f

fix broken link

5fc0500

Merge branch 'main' into reyang/metrics-sdk-view

971e770

jmacd approved these changes Jul 26, 2021

View reviewed changes

update the TODO part by removing the details

4ab3465

Merge branch 'main' into reyang/metrics-sdk-view

f9f47ed

jsuereth merged commit 872ff29 into open-telemetry:main Jul 27, 2021

reyang deleted the reyang/metrics-sdk-view branch July 27, 2021 17:26

reyang mentioned this pull request Aug 3, 2021

Remove MetricProcessor from the SDK spec (for now) #1840

Merged

reyang mentioned this pull request Aug 26, 2021

Add MetricReader interface #1888

Merged

jack-berg mentioned this pull request Feb 16, 2022

Clarify behavior when multiple views select the same instrument #2356

Closed

pellared mentioned this pull request May 31, 2023

Verify compliant metric SDK specification implementation: MeterProvider/Meter Creation open-telemetry/opentelemetry-go#3642

Closed

2 tasks

schmikei pushed a commit to schmikei/opentelemetry-specification that referenced this pull request Apr 17, 2025

Messaging: Align span kind in example tables with the spec (open-tele…

1aa6a5c

…metry#1730)

Conversation

reyang commented May 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsuereth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsuereth commented Jun 6, 2021

Uh oh!

Uh oh!

Uh oh!

jmacd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsuereth commented Jul 21, 2021

Uh oh!

Uh oh!

Uh oh!

jmacd left a comment

Choose a reason for hiding this comment

Uh oh!

reyang commented Jul 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

reyang commented May 28, 2021 •

edited

Loading