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.

Sign up for GitHub

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

Jump to bottom

MeterProvider and Meter API spec #1557

Merged

arminru merged 7 commits into open-telemetry:main from reyang:reyang/metrics-api

Mar 23, 2021

Member

reyang commented Mar 18, 2021 •

edited

Loading

Fixes #1046.
Fixes #1433.

Changes

Follow up on #1533 and the Metrics API/SDK SIG.

Reduced the scope of the metrics API spec by focusing on OTEP146 scenario.
Borrowed the idea from the stable version of Tracing API, organized the Metrics API spec in the similar flow.
Documented the MeterProvider and Meter API.
Added the initial skeleton of the Instrument API.

Related issues #1113 #1366

Related oteps

https://github.com/open-telemetry/oteps/blob/main/text/metrics/0146-metrics-prototype-scenarios.md

reyang requested review from a team

March 18, 2021 07:02

reyang added spec:metrics area:api labels

github-actions bot assigned arminru

MrAlias reviewed

View reviewed changes

specification/metrics/api.md Outdated Show resolved Hide resolved

specification/metrics/api.md Outdated Show resolved Hide resolved

bogdandrutu previously requested changes

View reviewed changes

Member

bogdandrutu left a comment

This is a huge change on how we envisioned metrics API and how we wanted this to be. I am not clear about motivation and direction of this project.

bogdandrutu reviewed

View reviewed changes

Member

bogdandrutu left a comment

Also I would encourage to create a new file for the moment "new_api.md" it is extremely hard to review since too many deletes and github does not help.

Member Author

reyang commented Mar 18, 2021

Also I would encourage to create a new file for the moment "new_api.md" it is extremely hard to review since too many deletes and github does not help.

Yes, we'll discuss this in today's Metrics API/SDK SIG meeting. Current options are:

use OTEP before making PR to spec repo.
create a separate doc (e.g. new_api.md).
change the current spec.

Member

bogdandrutu commented Mar 18, 2021

create a separate doc (e.g. new_api.md).

Asking because it is hard to read the new text, before merging we can discuss if we replace the old text, but cannot read what is the new text

jmacd approved these changes

View reviewed changes


          create a separate file

0fc42f0

reyang force-pushed the reyang/metrics-api branch from a10bd5e to 0fc42f0 Compare

March 19, 2021 02:28


          remove broken links

a76816c

Member Author

reyang commented Mar 19, 2021 •

edited

Loading

PTAL. Based on the discussion during the Metrics API/SDK SIG meeting, I've created a separate file (new_api.md) to make it easier for PR review, and removed gauge/histogram (which we will cover in separate PRs).

sirianni reviewed

View reviewed changes

specification/metrics/new_api.md

Comment on lines +164 to +166

+              * Different Meters MUST be treated as separate namespaces. The names of the
+                Instruments under one Meter SHOULD NOT interfere with Instruments under
+                another Meter.

Contributor

sirianni Mar 19, 2021 •

edited

Loading

@jmacd mentioned in Slack:

if you use the same metric name and different instrumentation libraries, the intention is that they are the same metric

However, unless I'm misreading, I don't see this explicitly stated here.

Contributor

jsuereth Mar 19, 2021

"are identified by their name". Maybe that need some highlights/implciations.

hdost Mar 22, 2021

"are identified by their name". Maybe that need some highlights/implciations.

I'm assuming this is related to: #1113

jsuereth approved these changes

View reviewed changes

specification/metrics/new_api.md Outdated Show resolved Hide resolved


          rewording

102f90e

bogdandrutu reviewed

View reviewed changes

specification/metrics/new_api.md Outdated Show resolved Hide resolved

specification/metrics/new_api.md Outdated Show resolved Hide resolved

specification/metrics/new_api.md Outdated Show resolved Hide resolved

bogdandrutu dismissed their stale review

March 20, 2021 01:46

Since the main section about gauge and histogram was removed no blocker

hdost reviewed

View reviewed changes

specification/metrics/new_api.md Outdated Show resolved Hide resolved

specification/metrics/new_api.md Show resolved Hide resolved

specification/metrics/new_api.md Show resolved Hide resolved


          address review comments

3e77ad7

bogdandrutu approved these changes

View reviewed changes

Member

bogdandrutu left a comment

Overall a good starting point

cijothomas approved these changes

View reviewed changes

reyang added 2 commits

March 22, 2021 14:48


          Merge branch 'main' into reyang/metrics-api

ea0836e


          avoid object-oriented wording

7be4588

reyang mentioned this pull request

The spec should not impose object-oriented paradigms #1569

Open

noahfalk reviewed

View reviewed changes

specification/metrics/new_api.md


		* [Create a new Counter](#counter-creation) (see the section on `Counter`)

		## Instrument

noahfalk Mar 23, 2021 •

edited

Loading

Do we need to say anything about the lifetime of these objects? I'm fine if lifetime is left as the language implementer's choice, or if the spec wants to declare something more specific.

Member Author

reyang Mar 23, 2021

I think it is fine to not specify the lifetime here (similar like the trace spec).

noahfalk Mar 24, 2021

OK I'm assuming anything not specified is implicitly implementer's choice : )

noahfalk reviewed

View reviewed changes

specification/metrics/new_api.md

+              ```text
+              +-- MeterProvider(default)
+                  |
+                  +-- Meter(name='io.opentelemetry.runtime', version='1.0.0')

noahfalk Mar 23, 2021

I know .NET might be a bit unique here. It's fine if MeterProvider has an API to create a Meter but we were intending that .NET libraries will also be able to create Meter directly (similar to ActivitySource), and that we would recommend them to do so. Thus on .NET MeterProvider needs to be able to subscribe to a Meter that already exists. I don't care whether the spec calls it out, but I want to make sure this isn't taking anyone by surprise in the design process.

Member Author

reyang Mar 23, 2021

This seems to be the same approach as what ActivitySource is doing for trace/span, so I don't see a problem here.


          Merge branch 'main' into reyang/metrics-api

88d89a0

arminru reviewed

View reviewed changes

specification/metrics/new_api.md

Comment on lines +5 to +13

+              **Owner:**
+              * [Reiley Yang](https://github.com/reyang)
+              **Domain Experts:**
+              * [Bogdan Brutu](https://github.com/bogdandrutu)
+              * [Josh Suereth](https://github.com/jsuereth)
+              * [Joshua MacDonald](https://github.com/jmacd)

Member

arminru Mar 23, 2021 •

edited

Loading

NB: This is questioned in #1503 and might be removed again.
cc @bogdandrutu

arminru merged commit d6d8dc4 into open-telemetry:main

ThomsonTan pushed a commit to ThomsonTan/opentelemetry-specification that referenced this pull request


          MeterProvider and Meter API spec (open-telemetry#1557)

bedfc8f

reyang mentioned this pull request

Rename "Label" concept to "Attribute" for consistency with OpenTelemetry tracing #1113

Closed

This was referenced May 5, 2021

Rename the metrics new_api.md to api.md and updated links #1672

Merged

Consolidate specification of global tracing and metric providers #613

Closed

rename api.md to old_api.md and update links (to point to new_api.md) #1674

Merged

reyang deleted the reyang/metrics-api branch

October 4, 2021 17:33

aabmass mentioned this pull request

Allow metric instrument names to contain / characters #3422

Closed

pellared mentioned this pull request

Meter name validation #3579

Closed

trask mentioned this pull request

Increase metric name maximum length from 63 to 255 characters #3648

Merged

carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request


          MeterProvider and Meter API spec (open-telemetry#1557)

15dcc4b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

arminru arminru left review comments

noahfalk noahfalk left review comments

MrAlias MrAlias left review comments

hdost hdost left review comments

sirianni sirianni left review comments

jsuereth jsuereth approved these changes

cijothomas cijothomas approved these changes

jmacd jmacd approved these changes

bogdandrutu bogdandrutu approved these changes

Labels

area:api spec:metrics