Skip to content
This repository was archived by the owner on Nov 17, 2025. It is now read-only.

OTEP 0049: LabelSet specification (to match current Metrics spec)#49

Merged
bogdandrutu merged 22 commits intoopen-telemetry:masterfrom
jmacd:jmacd/metricslabels2
Nov 16, 2019
Merged

OTEP 0049: LabelSet specification (to match current Metrics spec)#49
bogdandrutu merged 22 commits intoopen-telemetry:masterfrom
jmacd:jmacd/metricslabels2

Conversation

@jmacd
Copy link
Contributor

@jmacd jmacd commented Sep 13, 2019

This proposal was discussed in the Sept 10, 2019 metrics call.

@jmacd jmacd mentioned this pull request Sep 13, 2019
Merged
toumorokoshi
toumorokoshi reviewed Sep 16, 2019
View reviewed changes
lzchen
lzchen reviewed Sep 16, 2019
View reviewed changes
songy23
songy23 reviewed Sep 17, 2019
View reviewed changes
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

./text/0000-metric-label-set.md:97:310: "acceptible" is a misspelling of "acceptable"
make: *** [Makefile:16: misspell] Error 2

Please fix.

bogdandrutu
bogdandrutu reviewed Sep 18, 2019
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 looks like this proposal removes the Handle and replace it with raw APIs on metrics that accepts a LabelSet. I think this is too much statsd model centric and we penalize all the new modern systems/protocols like prometheus and openmetrics.

If we move forward with what we agreed that all the operations happens on a Handle (except the batch record) then the only place where this optimization may have an effect is on the batch.

Based on my experience when we used the batch version like grpc/http the labels are not known at compilation time, so the label set must be constructed for every individual request so this optimization does not have any effect (adds more overhead most likely because I need to create an extra object LabelSet).

I am not sure that we need this for the moment.

@jmacd jmacd mentioned this pull request Sep 20, 2019
Merged
@lzchen lzchen mentioned this pull request Sep 22, 2019
Merged
@tedsuo tedsuo added this to the Alpha v0.2 milestone Oct 8, 2019
@jmacd jmacd mentioned this pull request Oct 28, 2019
Merged
dyladan
dyladan reviewed Oct 28, 2019
View reviewed changes
jmacd and others added 5 commits October 28, 2019 13:17
@jmacd @dyladan
Update text/0049-metric-label-set.md
33288d4 
Co-Authored-By: dyladan <dyladan@users.noreply.github.com>
@jmacd @dyladan
Update text/0049-metric-label-set.md
f292c22 
Co-Authored-By: dyladan <dyladan@users.noreply.github.com>
@jmacd @dyladan
Update text/0049-metric-label-set.md
55ac75a 
Co-Authored-By: dyladan <dyladan@users.noreply.github.com>
Accept suggested phrasing
38e33a3
Revert mod changes eh?
31cef6b
MrAlias
MrAlias approved these changes Nov 4, 2019
View reviewed changes
@jmacd @MrAlias
Update text/0049-metric-label-set.md
9eab901 
Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>
iredelmeier
iredelmeier approved these changes Nov 8, 2019
View reviewed changes
Copy link
Member

@iredelmeier iredelmeier left a comment

Choose a reason for hiding this comment

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

LGTM overall.

I think it's worth making it very, very clear what is expected to live in the API and what will need to be re-implemeneted by each SDK.

@jmacd @iredelmeier
Update text/0049-metric-label-set.md
0e52002 
Co-Authored-By: Isobel Redelmeier <iredelmeier@gmail.com>
This was referenced Nov 12, 2019
Merged
Closed
bogdandrutu
bogdandrutu reviewed Nov 13, 2019
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.

LG overall, some comments left

text/0049-metric-label-set.md Outdated
In languages where overloading is a standard convenience, the metrics API may elect to offer alternate forms that elide the call to `Meter.Labels()`, for example:

```
instrument.GetHandle(meter, { Key: Value, ... })
Copy link
Member

@bogdandrutu bogdandrutu Nov 13, 2019

Choose a reason for hiding this comment

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

I think this needs to be updated because instrument is now generated by the meter so no need to pass that.

Copy link
Contributor Author

@jmacd jmacd Nov 13, 2019

Choose a reason for hiding this comment

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

Fixed.

@bogdandrutu
Copy link
Member

bogdandrutu commented Nov 13, 2019

One comment:
What is the behavior if a LabelSet is created by one Meter instance and the instrument was created by other Meter? This is in case of a named meter/tracer.

@jmacd
Copy link
Contributor Author

jmacd commented Nov 13, 2019

Your last question about named meters kind of made my head explode. I wish we had a standard term for the thing which is shared by all named meters, is that "provider", is that "engine", is that "SDK"? I would state that labels are usable by any meter from the same provider.

I'm adding this text

### Interaction with "Named" Meters

LabelSet values may be used with any named Meter originating from the
same Meter provider.  That is, LabelSets acquired through a named
Meter may be used by any Meter from the same Meter provider.

@bogdandrutu
Copy link
Member

bogdandrutu commented Nov 16, 2019

This PR was opened for a long time and it has 3 official approver LGTM + few others coming from people interested in metrics. Entering God mode and merge this.

@bogdandrutu bogdandrutu merged commit 7660052 into open-telemetry:master Nov 16, 2019
@MrAlias MrAlias mentioned this pull request Jan 7, 2020
Closed
6 tasks
@jmacd jmacd deleted the jmacd/metricslabels2 branch May 6, 2020 18:48
carlosalberto pushed a commit to carlosalberto/oteps that referenced this pull request Oct 23, 2024
@jmacd @bogdandrutu
OTEP 0049: LabelSet specification (to match current Metrics spec) (op…
7be051e 
…en-telemetry#49)

* Draft LabelSet spec

* Typos

* Add notes on performance expectations

* Typo

* Updates to use Monotonic/NonMonotonic, and NonNegtive/Signed

* Revert

* PR number 49

* Major revision to match the already-merged specification on LabelSet

* Update text/0049-metric-label-set.md

Co-Authored-By: dyladan <dyladan@users.noreply.github.com>

* Update text/0049-metric-label-set.md

Co-Authored-By: dyladan <dyladan@users.noreply.github.com>

* Update text/0049-metric-label-set.md

Co-Authored-By: dyladan <dyladan@users.noreply.github.com>

* Accept suggested phrasing

* Revert mod changes eh?

* Update text/0049-metric-label-set.md

Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>

* Update text/0049-metric-label-set.md

Co-Authored-By: Isobel Redelmeier <iredelmeier@gmail.com>

* Remove explicit meter argument where not necessary
carlosalberto pushed a commit to carlosalberto/oteps that referenced this pull request Oct 23, 2024
@jmacd @bogdandrutu
OTEP 0049: LabelSet specification (to match current Metrics spec) (op…
c29d46d 
…en-telemetry#49)

* Draft LabelSet spec

* Typos

* Add notes on performance expectations

* Typo

* Updates to use Monotonic/NonMonotonic, and NonNegtive/Signed

* Revert

* PR number 49

* Major revision to match the already-merged specification on LabelSet

* Update text/0049-metric-label-set.md

Co-Authored-By: dyladan <dyladan@users.noreply.github.com>

* Update text/0049-metric-label-set.md

Co-Authored-By: dyladan <dyladan@users.noreply.github.com>

* Update text/0049-metric-label-set.md

Co-Authored-By: dyladan <dyladan@users.noreply.github.com>

* Accept suggested phrasing

* Revert mod changes eh?

* Update text/0049-metric-label-set.md

Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>

* Update text/0049-metric-label-set.md

Co-Authored-By: Isobel Redelmeier <iredelmeier@gmail.com>

* Remove explicit meter argument where not necessary
carlosalberto pushed a commit to carlosalberto/oteps that referenced this pull request Oct 30, 2024
@jmacd @bogdandrutu
OTEP 0049: LabelSet specification (to match current Metrics spec) (op…
905e355 
…en-telemetry#49)

* Draft LabelSet spec

* Typos

* Add notes on performance expectations

* Typo

* Updates to use Monotonic/NonMonotonic, and NonNegtive/Signed

* Revert

* PR number 49

* Major revision to match the already-merged specification on LabelSet

* Update text/0049-metric-label-set.md

Co-Authored-By: dyladan <dyladan@users.noreply.github.com>

* Update text/0049-metric-label-set.md

Co-Authored-By: dyladan <dyladan@users.noreply.github.com>

* Update text/0049-metric-label-set.md

Co-Authored-By: dyladan <dyladan@users.noreply.github.com>

* Accept suggested phrasing

* Revert mod changes eh?

* Update text/0049-metric-label-set.md

Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>

* Update text/0049-metric-label-set.md

Co-Authored-By: Isobel Redelmeier <iredelmeier@gmail.com>

* Remove explicit meter argument where not necessary
carlosalberto pushed a commit to open-telemetry/opentelemetry-specification that referenced this pull request Nov 8, 2024
@jmacd @carlosalberto
OTEP 0049: LabelSet specification (to match current Metrics spec) (op…
0849e14 
…en-telemetry/oteps#49)

* Draft LabelSet spec

* Typos

* Add notes on performance expectations

* Typo

* Updates to use Monotonic/NonMonotonic, and NonNegtive/Signed

* Revert

* PR number 49

* Major revision to match the already-merged specification on LabelSet

* Update text/0049-metric-label-set.md

Co-Authored-By: dyladan <dyladan@users.noreply.github.com>

* Update text/0049-metric-label-set.md

Co-Authored-By: dyladan <dyladan@users.noreply.github.com>

* Update text/0049-metric-label-set.md

Co-Authored-By: dyladan <dyladan@users.noreply.github.com>

* Accept suggested phrasing

* Revert mod changes eh?

* Update text/0049-metric-label-set.md

Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>

* Update text/0049-metric-label-set.md

Co-Authored-By: Isobel Redelmeier <iredelmeier@gmail.com>

* Remove explicit meter argument where not necessary
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AloisReitbauer AloisReitbauer Awaiting requested review from AloisReitbauer

@c24t c24t Awaiting requested review from c24t

@carlosalberto carlosalberto Awaiting requested review from carlosalberto

@reyang reyang Awaiting requested review from reyang

@SergeyKanzhelev SergeyKanzhelev Awaiting requested review from SergeyKanzhelev

@tedsuo tedsuo Awaiting requested review from tedsuo

@yurishkuro yurishkuro Awaiting requested review from yurishkuro

8 more reviewers

@toumorokoshi toumorokoshi toumorokoshi left review comments

@songy23 songy23 songy23 left review comments

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@dyladan dyladan dyladan approved these changes

@iredelmeier iredelmeier iredelmeier approved these changes

@tigrannajaryan tigrannajaryan tigrannajaryan approved these changes

@MrAlias MrAlias MrAlias approved these changes

@lzchen lzchen lzchen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Alpha v0.2

Development

Successfully merging this pull request may close these issues.

10 participants

@jmacd @tigrannajaryan @bogdandrutu @toumorokoshi @dyladan @iredelmeier @MrAlias @songy23 @lzchen @tedsuo