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.

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
merged 7 commits into from
Mar 23, 2021
Merged

MeterProvider and Meter API spec #1557

merged 7 commits into from
Mar 23, 2021

Conversation

reyang
Copy link
Member

@reyang 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

@reyang reyang requested review from a team March 18, 2021 07:02
@reyang reyang added spec:metrics Related to the specification/metrics directory area:api Cross language API specification issue labels Mar 18, 2021
MrAlias
MrAlias reviewed Mar 18, 2021
View reviewed changes
specification/metrics/api.md Outdated Show resolved Hide resolved
specification/metrics/api.md Outdated Show resolved Hide resolved
bogdandrutu
bogdandrutu previously requested changes Mar 18, 2021
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
bogdandrutu reviewed Mar 18, 2021
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@reyang
Copy link
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:

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

@bogdandrutu
Copy link
Member

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

@reyang
Copy link
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
sirianni reviewed Mar 19, 2021
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.
Copy link
Contributor

@sirianni sirianni Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I'm assuming this is related to: #1113

jsuereth
jsuereth approved these changes Mar 19, 2021
View reviewed changes
specification/metrics/new_api.md Outdated Show resolved Hide resolved
bogdandrutu
bogdandrutu reviewed Mar 20, 2021
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 bogdandrutu dismissed their stale review March 20, 2021 01:46

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

hdost
hdost reviewed Mar 22, 2021
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
bogdandrutu
bogdandrutu approved these changes Mar 22, 2021
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall a good starting point

@reyang reyang mentioned this pull request Mar 22, 2021
Open
noahfalk
noahfalk reviewed Mar 23, 2021
View reviewed changes
specification/metrics/new_api.md

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

## Instrument
Copy link

@noahfalk noahfalk Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

noahfalk
noahfalk reviewed Mar 23, 2021
View reviewed changes
specification/metrics/new_api.md
```text
+-- MeterProvider(default)
|
+-- Meter(name='io.opentelemetry.runtime', version='1.0.0')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

arminru
arminru reviewed Mar 23, 2021
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)
Copy link
Member

@arminru arminru Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@arminru arminru merged commit d6d8dc4 into open-telemetry:main Mar 23, 2021
ThomsonTan pushed a commit to ThomsonTan/opentelemetry-specification that referenced this pull request Mar 30, 2021
@reyang @ThomsonTan
MeterProvider and Meter API spec (open-telemetry#1557)
bedfc8f
@reyang reyang mentioned this pull request Apr 23, 2021
Closed
This was referenced May 5, 2021
Merged
Closed
Merged
@reyang reyang deleted the reyang/metrics-api branch October 4, 2021 17:33
@aabmass aabmass mentioned this pull request Apr 20, 2023
Closed
@pellared pellared mentioned this pull request Jul 25, 2023
Closed
@trask trask mentioned this pull request Aug 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsuereth jsuereth jsuereth approved these changes

@sirianni sirianni sirianni left review comments

@hdost hdost hdost left review comments

@MrAlias MrAlias MrAlias left review comments

@noahfalk noahfalk noahfalk left review comments

@arminru arminru arminru left review comments

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@jmacd jmacd jmacd approved these changes

@cijothomas cijothomas cijothomas approved these changes

Assignees

@arminru arminru

Labels
area:api Cross language API specification issue spec:metrics Related to the specification/metrics directory
Projects
None yet
Milestone
No milestone
10 participants
@reyang @bogdandrutu @jsuereth @sirianni @hdost @jmacd @cijothomas @MrAlias @noahfalk @arminru