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

jmacd · 2019-09-13T23:33:42Z

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

text/0000-metric-label-set.md

songy23

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

Please fix.

bogdandrutu

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.

text/0000-metric-label-set.md

text/0049-metric-label-set.md

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

text/0049-metric-label-set.md

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

iredelmeier

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.

text/0049-metric-label-set.md

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

bogdandrutu

LG overall, some comments left

text/0049-metric-label-set.md

bogdandrutu · 2019-11-13T23:08:01Z

text/0049-metric-label-set.md

+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, ... })


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

text/0049-metric-label-set.md

bogdandrutu · 2019-11-13T23:10:23Z

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 · 2019-11-13T23:26:54Z

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 · 2019-11-16T19:13:00Z

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.

…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

…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

jmacd added 2 commits September 13, 2019 16:25

Draft LabelSet spec

bdd3431

Typos

55488a5

jmacd requested review from AloisReitbauer, bogdandrutu, c24t, carlosalberto, iredelmeier, reyang, SergeyKanzhelev, songy23, tedsuo and yurishkuro as code owners September 13, 2019 23:33

jmacd mentioned this pull request Sep 13, 2019

Golang metrics prototype open-telemetry/opentelemetry-go#100

Merged

toumorokoshi reviewed Sep 16, 2019

View reviewed changes

text/0000-metric-label-set.md Outdated Show resolved Hide resolved

Add notes on performance expectations

5037a48

lzchen reviewed Sep 16, 2019

View reviewed changes

text/0000-metric-label-set.md Outdated Show resolved Hide resolved

lzchen approved these changes Sep 17, 2019

View reviewed changes

songy23 reviewed Sep 17, 2019

View reviewed changes

jmacd added 2 commits September 17, 2019 13:25

Upstream

cb5f69c

Typo

6713f93

bogdandrutu reviewed Sep 18, 2019

View reviewed changes

text/0000-metric-label-set.md Outdated Show resolved Hide resolved

jmacd added 2 commits September 18, 2019 14:41

Updates to use Monotonic/NonMonotonic, and NonNegtive/Signed

bb51cca

Revert

2884dcf

jmacd mentioned this pull request Sep 20, 2019

Metrics specification updates open-telemetry/opentelemetry-specification#250

Merged

lzchen mentioned this pull request Sep 22, 2019

Metrics Implementation open-telemetry/opentelemetry-python#160

Merged

Merge branch 'master' into jmacd/metricslabels2

16343fb

bogdandrutu requested a review from tigrannajaryan as a code owner October 8, 2019 15:15

tedsuo added this to the Alpha v0.2 milestone Oct 8, 2019

jmacd added 2 commits October 21, 2019 20:36

Upstream

1e1261c

PR number 49

fe54cd8

jmacd mentioned this pull request Oct 28, 2019

Update 0009-metric-handles to match current spec #62

Merged

lzchen approved these changes Oct 28, 2019

View reviewed changes

dyladan reviewed Oct 28, 2019

View reviewed changes

text/0049-metric-label-set.md Outdated Show resolved Hide resolved

text/0049-metric-label-set.md Outdated Show resolved Hide resolved

text/0049-metric-label-set.md Outdated Show resolved Hide resolved

text/0049-metric-label-set.md Outdated Show resolved Hide resolved

jmacd and others added 5 commits October 28, 2019 13:17

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

33288d4

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

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

f292c22

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

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

dyladan approved these changes Oct 28, 2019

View reviewed changes

MrAlias approved these changes Nov 4, 2019

View reviewed changes

text/0049-metric-label-set.md Outdated Show resolved Hide resolved

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

9eab901

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

iredelmeier approved these changes Nov 8, 2019

View reviewed changes

text/0049-metric-label-set.md Outdated Show resolved Hide resolved

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

0e52002

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

This was referenced Nov 12, 2019

Metric SDK specification OUTLINE open-telemetry/opentelemetry-specification#347

Merged

[api-metrics] Mention of 'context' w/o definition. open-telemetry/opentelemetry-specification#352

Closed

Merge branch 'master' into jmacd/metricslabels2

ba93e34

bogdandrutu reviewed Nov 13, 2019

View reviewed changes

Remove explicit meter argument where not necessary

1ccd9ed

Merge

af81183

bogdandrutu approved these changes Nov 14, 2019

View reviewed changes

bogdandrutu merged commit 7660052 into open-telemetry:master Nov 16, 2019

MrAlias mentioned this pull request Jan 7, 2020

REQUEST: New membership for MrAlias open-telemetry/community#281

Closed

6 tasks

jmacd deleted the jmacd/metricslabels2 branch May 6, 2020 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jmacd commented Sep 13, 2019

songy23 left a comment

bogdandrutu left a comment

iredelmeier left a comment

bogdandrutu left a comment

bogdandrutu Nov 13, 2019

jmacd Nov 13, 2019

bogdandrutu commented Nov 13, 2019

jmacd commented Nov 13, 2019

bogdandrutu commented Nov 16, 2019

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

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

Conversation

jmacd commented Sep 13, 2019

songy23 left a comment

Choose a reason for hiding this comment

bogdandrutu left a comment

Choose a reason for hiding this comment

iredelmeier left a comment

Choose a reason for hiding this comment

bogdandrutu left a comment

Choose a reason for hiding this comment

bogdandrutu Nov 13, 2019

Choose a reason for hiding this comment

jmacd Nov 13, 2019

Choose a reason for hiding this comment

bogdandrutu commented Nov 13, 2019

jmacd commented Nov 13, 2019

bogdandrutu commented Nov 16, 2019