skeleton(metrics): createMeasure #563

markwolff · 2019-11-22T18:52:30Z

Which problem is this PR solving?

Closes (Metrics) Implement "createMeasure" #452
Updates Implement Metrics SDK #402

Short description of the changes

Skeleton implementation of createMeasure

TODOs

Implement data handling (histogramming, etc) for Measure.record (separate PR)

codecov-io · 2019-11-22T18:58:17Z

Codecov Report

❗ No coverage uploaded for pull request base (master@9a67900). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #563   +/-   ##
=========================================
  Coverage          ?   91.08%           
=========================================
  Files             ?      231           
  Lines             ?    10474           
  Branches          ?      968           
=========================================
  Hits              ?     9540           
  Misses            ?      934           
  Partials          ?        0

Impacted Files	Coverage Δ
...metry-core/src/trace/instrumentation/BasePlugin.ts	`86.84% <ø> (ø)`
...ry-plugin-dns/test/functionals/dns-disable.test.ts	`100% <ø> (ø)`
packages/opentelemetry-api/src/trace/status.ts	`100% <ø> (ø)`
...entelemetry-exporter-jaeger/test/transform.test.ts	`100% <ø> (ø)`
...telemetry-plugin-document-load/src/documentLoad.ts	`97.95% <ø> (ø)`
...s/opentelemetry-plugin-xml-http-request/src/xhr.ts	`79.41% <ø> (ø)`
...lugin-https/test/functionals/https-disable.test.ts	`100% <ø> (ø)`
...s/opentelemetry-metrics/test/MeterProvider.test.ts	`100% <ø> (ø)`
...ges/opentelemetry-exporter-zipkin/src/transform.ts	`100% <ø> (ø)`
...metry-core/src/trace/sampler/ProbabilitySampler.ts	`100% <ø> (ø)`
... and 125 more

packages/opentelemetry-metrics/src/Handle.ts

packages/opentelemetry-metrics/src/Meter.ts

mayurkale22 · 2019-11-22T20:57:15Z

packages/opentelemetry-metrics/src/types.ts

@@ -40,6 +40,9 @@ export interface MetricOptions {
  /** Monotonic allows this metric to accept negative values. */
  monotonic: boolean;

+  /** (Measure only) Absolute asserts that this metric will only accept positive values. Default true */
+  absolute?: boolean;


For me, monotonic and absolute flags looks almost the same.

I agree. Even though the spec calls for absolute for measures, I wonder if it should be proposed to reuse the same config name for all metrics? The only difference between them is that their effects are "reversed"

I think we should open issue against specs and ask for clarification. It is highly encouraged to escalate issues on top level. WDYT?

SGTM. open-telemetry/opentelemetry-specification#364

IMO absolute and monotonic don't mean the same thing at all. absolute means non-negative, monotonic means only counts up. An absolute, non monotonic measure may measure something like disk usage which can go up and down but never be below zero.

edit: also made this comment the issue you created

@markwolff can you add the @dyladan explanation into the jsdoc? Currently both descriptions sounds similar for me and it is hard to understand the difference between them.

Per spec open-telemetry/opentelemetry-specification#364 we will keep absolute and monotonic as they mean different things.

Suggest JSDoc:

Absolute: Asserts that this metric may only have a positive value (e.g. disk usage)

Monotonic: Asserts that this metric may only increase (e.g. time spent)

packages/opentelemetry-metrics/src/Metric.ts

mayurkale22 · 2019-11-22T23:31:33Z

packages/opentelemetry-metrics/src/Metric.ts

+        : MetricDescriptorType.GAUGE_INT64
+    );
+
+    this._absolute = options.absolute !== undefined ? options.absolute : true; // Absolute default is true


Another approach, what if we make absolute as a mandatory attribute in SDK (opentelemetry-metrics/src/types.ts) similar to other attributes. Most of these attributes are optional in opentelemetry-types but to make life easier, we made it mandatory in the SDK with default values.

packages/opentelemetry-metrics/src/Metric.ts

packages/opentelemetry-types/src/metrics/Metric.ts

packages/opentelemetry-metrics/src/Metric.ts

dyladan · 2019-11-27T18:50:58Z

packages/opentelemetry-metrics/src/types.ts

@@ -40,6 +40,9 @@ export interface MetricOptions {
  /** Monotonic allows this metric to accept negative values. */
  monotonic: boolean;

+  /** (Measure only) Absolute asserts that this metric will only accept positive values. Default true */
+  absolute?: boolean;


Per spec open-telemetry/opentelemetry-specification#364 we will keep absolute and monotonic as they mean different things.

Suggest JSDoc:

Absolute: Asserts that this metric may only have a positive value (e.g. disk usage)

Monotonic: Asserts that this metric may only increase (e.g. time spent)

packages/opentelemetry-metrics/src/types.ts

packages/opentelemetry-types/src/metrics/Metric.ts

dyladan · 2020-01-02T20:44:33Z

packages/opentelemetry-metrics/src/Handle.ts

@@ -130,12 +130,44 @@ export class GaugeHandle extends BaseHandle implements types.GaugeHandle {
 * MeasureHandle is an implementation of the {@link MeasureHandle} interface.
 */
 export class MeasureHandle extends BaseHandle implements types.MeasureHandle {
+  constructor(
+    labelSet: types.LabelSet,
+    private readonly _disabled: boolean,


if something is readonly and disabled, is there no way to enable it? How is this different than noop

It seems like it is just a way for sdk to create noop handles via ctor, but I'm not sure if there are any benefits to it over just having NoopHandle or something else. It is a lot less code with just _disabled! Maybe @mayurkale22 would know?

xiao-lix · 2020-01-28T22:24:22Z

packages/opentelemetry-metrics/test/Meter.test.ts

+    describe('.bind()', () => {
+      it('should create a measure handle', () => {
+        const measure = meter.createMeasure('name') as MeasureMetric;
+        const handle = measure.bind(labelSet);


nit: update name handle to bind?

I think boundMeasure would make more sense

@markwolff Could you please fix last round of comments and resolve the conflicts? Looks good to go after that.

Rebased and addressed this comment 8e53f1e

dyladan · 2020-01-29T00:35:57Z

packages/opentelemetry-metrics/src/BoundInstrument.ts

+      value = Math.trunc(value);
+    }
+
+    //@todo: implement this._data logic


Does this need to be done before merge?

I dont think so, I am gonna update this in my PR(#738) once this is merged.

mayurkale22 · 2020-01-29T00:56:04Z

packages/opentelemetry-metrics/src/Metric.ts

@@ -199,3 +200,38 @@ export class GaugeMetric extends Metric<BoundGauge>
    this.bind(labelSet).set(value);
  }
 }
+
+export class MeasureMetric extends Metric<BoundMeasure>
+  implements Pick<types.MetricUtils, 'record'> {


I think in the future, we have to include recordBatch API, which will provide users a convenient way to add multiple events/measurements once.

Is that in your PR?

No. We can include it later once the initial pipeline is working.

mayurkale22

lgtm

* feat: implement createMeasure * fix: linting * fix: disable broken tests * fix: labelSet log statement * fix: absolute always set to true * fix: linting * fix: add jsdoc, adjust optionals * fix: failing tests * fix: linting * refactor: rename test handle var Co-authored-by: Mayur Kale <[email protected]>

Mark Wolff added 2 commits November 22, 2019 10:44

feat: implement createMeasure

14e43ad

fix: linting

15db392

markwolff requested review from bg451, danielkhan, mayurkale22, obecny, OlivierAlbertini, rochdev and vmarchaud as code owners November 22, 2019 18:52

fix: disable broken tests

00e32f2

xiao-lix reviewed Nov 22, 2019

View reviewed changes

packages/opentelemetry-metrics/src/Handle.ts Outdated Show resolved Hide resolved

fix: labelSet log statement

0bf9c25

xiao-lix reviewed Nov 22, 2019

View reviewed changes

packages/opentelemetry-metrics/src/Meter.ts Show resolved Hide resolved

mayurkale22 reviewed Nov 22, 2019

View reviewed changes

Mark Wolff added 2 commits November 22, 2019 13:18

fix: absolute always set to true

f8dd8b6

fix: linting

78bea1b

mayurkale22 reviewed Nov 22, 2019

View reviewed changes

mayurkale22 reviewed Nov 23, 2019

View reviewed changes

packages/opentelemetry-metrics/src/Metric.ts Outdated Show resolved Hide resolved

obecny reviewed Nov 27, 2019

View reviewed changes

packages/opentelemetry-types/src/metrics/Metric.ts Outdated Show resolved Hide resolved

dyladan reviewed Nov 27, 2019

View reviewed changes

mayurkale22 added the Awaiting reviewer feedback label Nov 27, 2019

fix: add jsdoc, adjust optionals

5c18d1c

markwolff changed the title ~~feat(metrics): Implement createMeasure~~ skeleton(metrics): createMeasure Jan 2, 2020

dyladan reviewed Jan 2, 2020

View reviewed changes

markwolff added 4 commits January 13, 2020 11:57

Merge branch 'master' into markwolff/impl_createMeasure

6ad8f52

fix: failing tests

327f828

fix: linting

68ac62d

Merge branch 'master' into markwolff/impl_createMeasure

cf5e729

xiao-lix reviewed Jan 28, 2020

View reviewed changes

xiao-lix approved these changes Jan 28, 2020

View reviewed changes

mayurkale22 mentioned this pull request Jan 28, 2020

feat: Metrics SDK - aggregator, batcher, controller #738

Merged

Merge branch 'master' into markwolff/impl_createMeasure

af5d223

dyladan reviewed Jan 29, 2020

View reviewed changes

mayurkale22 reviewed Jan 29, 2020

View reviewed changes

dyladan approved these changes Jan 29, 2020

View reviewed changes

Mark Wolff added 3 commits February 3, 2020 12:34

Merge branch 'master' into markwolff/impl_createMeasure

9a67900

Merge branch 'master' into markwolff/impl_createMeasure

fcb9734

refactor: rename test handle var

8e53f1e

mayurkale22 approved these changes Feb 3, 2020

View reviewed changes

mayurkale22 added Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) and removed Awaiting reviewer feedback labels Feb 3, 2020

Merge branch 'master' into markwolff/impl_createMeasure

ce7552e

mayurkale22 merged commit 470fc62 into open-telemetry:master Feb 5, 2020

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023

docs(auto-instrumentations-node): improved readme (open-telemetry#563)

83c7fa3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

skeleton(metrics): createMeasure #563

skeleton(metrics): createMeasure #563

markwolff commented Nov 22, 2019 •

edited

Loading

codecov-io commented Nov 22, 2019 •

edited

Loading

mayurkale22 Nov 22, 2019

markwolff Nov 22, 2019

mayurkale22 Nov 22, 2019

markwolff Nov 22, 2019

dyladan Nov 23, 2019 •

edited

Loading

obecny Nov 27, 2019

dyladan Nov 27, 2019

mayurkale22 Nov 22, 2019

dyladan Nov 27, 2019

dyladan Jan 2, 2020

markwolff Jan 3, 2020

xiao-lix Jan 28, 2020 •

edited

Loading

dyladan Jan 28, 2020

mayurkale22 Feb 3, 2020

markwolff Feb 3, 2020

dyladan Jan 29, 2020

mayurkale22 Jan 29, 2020

mayurkale22 Jan 29, 2020

dyladan Jan 29, 2020 •

edited

Loading

mayurkale22 Jan 29, 2020

mayurkale22 left a comment

skeleton(metrics): createMeasure #563

skeleton(metrics): createMeasure #563

Conversation

markwolff commented Nov 22, 2019 • edited Loading

Which problem is this PR solving?

Short description of the changes

TODOs

codecov-io commented Nov 22, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan Nov 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiao-lix Jan 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan Jan 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 left a comment

Choose a reason for hiding this comment

markwolff commented Nov 22, 2019 •

edited

Loading

codecov-io commented Nov 22, 2019 •

edited

Loading

dyladan Nov 23, 2019 •

edited

Loading

xiao-lix Jan 28, 2020 •

edited

Loading

dyladan Jan 29, 2020 •

edited

Loading