feat: implement metrics sdk #415

mayurkale22 · 2019-10-09T00:24:15Z

Which problem is this PR solving?

Updates Implement Metrics SDK #402
First take on metrics SDK. In order to keep short PR, only Counter metric support is added.
The monotonic concept is based on https://github.com/open-telemetry/opentelemetry-specification/pull/250/files.
i.e. Counters are defined as monotonic by default, meaning that positive values are expected and Gauges are defined as non-monotonic by default, meaning that any value (positive or negative) is allowed.
Record call is still missing, will do it in the follow-up PR.

Update 1: Added support for Gauge Metric.
Update 2: Added MeterConfig with Logger option.

packages/opentelemetry-metrics/src/Metric.ts

packages/opentelemetry-metrics/src/Handle.ts

draffensperger · 2019-10-09T15:09:37Z

packages/opentelemetry-metrics/src/Handle.ts

+    distContext?: types.DistributedContext,
+    spanContext?: types.SpanContext
+  ): void {
+    throw new Error('not implemented yet');


Should this just be a no-op since we are saying we don't through errors? (Or maybe log instead?)

packages/opentelemetry-metrics/src/Meter.ts

draffensperger · 2019-10-09T15:11:55Z

packages/opentelemetry-metrics/src/Meter.ts

+    name: string,
+    options?: types.MetricOptions
+  ): Metric<GaugeHandle> {
+    throw new Error('not implemented yet');


packages/opentelemetry-metrics/src/Metric.ts

codecov-io · 2019-10-09T18:15:27Z

Codecov Report

Merging #415 into master will increase coverage by 0.55%.
The diff coverage is 96.03%.

@@            Coverage Diff             @@
##           master     #415      +/-   ##
==========================================
+ Coverage   95.33%   95.89%   +0.55%     
==========================================
  Files         118      114       -4     
  Lines        5852     5599     -253     
  Branches      502      440      -62     
==========================================
- Hits         5579     5369     -210     
+ Misses        273      230      -43

Impacted Files	Coverage Δ
packages/opentelemetry-metrics/test/Meter.test.ts	`100% <100%> (ø)`
packages/opentelemetry-metrics/src/types.ts	`100% <100%> (ø)`
packages/opentelemetry-metrics/src/Utils.ts	`100% <100%> (ø)`
packages/opentelemetry-metrics/src/Metric.ts	`82.35% <82.35%> (ø)`
packages/opentelemetry-metrics/src/Meter.ts	`85.71% <85.71%> (ø)`
packages/opentelemetry-metrics/src/Handle.ts	`93.93% <93.93%> (ø)`
...opentelemetry-base/test/resources/resource.test.ts	`100% <0%> (ø)`	⬆️
...telemetry-plugin-grpc/test/utils/assertionUtils.ts	`100% <0%> (ø)`	⬆️
...pe-async-hooks/test/AsyncHooksScopeManager.test.ts	`100% <0%> (ø)`	⬆️
packages/opentelemetry-plugin-dns/src/dns.ts
... and 15 more

markwolff · 2019-10-10T16:47:52Z

packages/opentelemetry-metrics/src/Meter.ts

+    name: string,
+    options?: types.MetricOptions
+  ): Metric<CounterHandle> {
+    const opt = Object.assign({ monotonic: false, disabled: false }, options);


Is { monotonic: false, disabled: false } meant to be the default options? Would it make sense to move it to a DEFAULT_COUNTER_OPTIONS const?

Unidirectional needs to be renamed, but monotonic can come from the MetricsOptions: https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-types/src/metrics/Metric.ts#L60

packages/opentelemetry-metrics/src/Metric.ts

draffensperger · 2019-10-10T17:44:04Z

packages/opentelemetry-metrics/src/Handle.ts

+import * as types from '@opentelemetry/types';
+
+/**
+ * CounterHandle is an implementation of the {@link CounterHandle} interface.


This comment in itself doesn't seem too useful. I'm OK with removing it if needed. (Same applies for comment below)

I guess I was thinking this would instead talk a bit about what a counter is and what a handle is, but maybe the expectation doesn't make sense - perhaps people should just be reading the READMEs anyway.

Hmm, tried to update JSDoc comment in e0dcecd. Let me know if this makes sense.

dyladan · 2019-10-10T18:53:47Z

@mayurkale22 fwiw when you force push amendments the PR loses history and I have a hard time seeing what is changed from the previous version. Can you try to keep a history during the PR process and just flatten it before merge? Unless there is some other reason to keep it a single commit.

mayurkale22 · 2019-10-10T19:31:37Z

@mayurkale22 fwiw when you force push amendments the PR loses history and I have a hard time seeing what is changed from the previous version. Can you try to keep a history during the PR process and just flatten it before merge? Unless there is some other reason to keep it a single commit.

I didn't squash the previous comments, commits history is still intact. I had to do force push after rebasing with the master. Anyway, in general I agree with your suggestion.

dyladan · 2019-10-10T22:08:06Z

packages/opentelemetry-metrics/src/Metric.ts

+import { MetricOptions } from './types';
+
+/** This is a SDK implementation of {@link Metric} interface. */
+export abstract class Metric<T> implements types.Metric<T> {


dyladan · 2019-10-10T22:18:53Z

packages/opentelemetry-metrics/src/Metric.ts

+   * calling this method for each operation.
+   * @param labelValues the list of label values.
+   */
+  getHandle(labelValues: string[]): T {


I think getHandle isn't meant to take labelValues, but an instance of a LabelSet and just be a wrapper around a specific metric.

const labelSet1: LabelSet = meter.defineLabels(labels1) const labelSet2: LabelSet = meter.defineLabels(labels2) const counter = meter.createCounter("name"); counter.add(labelSet1, 1); // counter now has 1 for labelSet1 const handle = counter.getHandle(labelSet2); handle.add(1) // counter has 1 for labelSet2

dyladan · 2019-10-10T22:20:21Z

There were some discussions related to metrics today in the spec and I think it might be in flux. It's hard to tell how they're intending Metric/Handle/Meter to play together. It may be worth taking a look.

bg451 · 2019-10-11T15:50:39Z

packages/opentelemetry-metrics/src/Metric.ts

+    const hash = hashLabelValues(labelValues);
+    if (this._handles.has(hash)) return this._handles.get(hash)!;
+
+    const handle = this._makeHandle();


makeHandle, and the handles, should accept the label set/values

Hmm, correct. Done in 7d53d02. Also, added a method to generate TimeSeries from the handle's data + label values.

@bg451 I have made the required changes, please take a look again when you get a time.

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

mayurkale22 · 2019-10-16T22:24:19Z

Per the SIG, we agreed to merge this one and revise later based on Prometheus exporter and end-to-end workflow.

…ry#415) * fix: empty spans when encode failed in HTTPSender Signed-off-by: himstone.yang <himstone.yang@gmail.com> * Apply fixes Signed-off-by: Yuri Shkuro <ys@uber.com> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>

mayurkale22 requested review from bg451, danielkhan, iredelmeier, OlivierAlbertini, rochdev and vmarchaud as code owners October 9, 2019 00:24

mayurkale22 commented Oct 9, 2019

View reviewed changes

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

draffensperger reviewed Oct 9, 2019

View reviewed changes

mayurkale22 force-pushed the metrics_sdk branch from b67f3d0 to 3dcf180 Compare October 9, 2019 17:52

markwolff reviewed Oct 10, 2019

View reviewed changes

mayurkale22 force-pushed the metrics_sdk branch from 980f29a to c1bc52c Compare October 10, 2019 17:38

draffensperger reviewed Oct 10, 2019

View reviewed changes

dyladan reviewed Oct 10, 2019

View reviewed changes

bg451 reviewed Oct 11, 2019

View reviewed changes

mayurkale22 force-pushed the metrics_sdk branch from 71db9b0 to 2ed9b68 Compare October 14, 2019 21:19

bg451 approved these changes Oct 15, 2019

View reviewed changes

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

draffensperger approved these changes Oct 15, 2019

View reviewed changes

danielkhan approved these changes Oct 16, 2019

View reviewed changes

mayurkale22 force-pushed the metrics_sdk branch from 171d790 to 5dd40b9 Compare October 16, 2019 18:30

mayurkale22 added 5 commits October 16, 2019 14:45

feat: add metrics sdk

ec33120

refactor: based on comments and add Gauge support

95d1a06

fix: add JSDoc comment

2cc653f

fix: pass label values to handle and generate TimeSeries

42baa45

fix: add MeterConfig and logger option

6b3ee5f

mayurkale22 force-pushed the metrics_sdk branch from 5dd40b9 to 6b3ee5f Compare October 16, 2019 21:45

mayurkale22 merged commit a025108 into open-telemetry:master Oct 16, 2019

mayurkale22 deleted the metrics_sdk branch October 16, 2019 22:42

mayurkale22 mentioned this pull request Oct 17, 2019

feat(metrics): add registerMetric and getMetrics #437

Merged

mayurkale22 mentioned this pull request Oct 29, 2019

feat: implement labelset #463

Merged

mayurkale22 mentioned this pull request Jun 30, 2020

Batch observer #1137

Merged

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

feat(instrumentation-ioredis): add request hook (open-telemetry#415)

550e32c

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement metrics sdk #415

feat: implement metrics sdk #415

mayurkale22 commented Oct 9, 2019 •

edited

Loading

draffensperger Oct 9, 2019

draffensperger Oct 9, 2019

codecov-io commented Oct 9, 2019 •

edited

Loading

markwolff Oct 10, 2019

bg451 Oct 10, 2019

draffensperger Oct 10, 2019

mayurkale22 Oct 10, 2019

dyladan commented Oct 10, 2019

mayurkale22 commented Oct 10, 2019

dyladan Oct 10, 2019

dyladan Oct 10, 2019

dyladan commented Oct 10, 2019

bg451 Oct 11, 2019

mayurkale22 Oct 11, 2019

mayurkale22 Oct 14, 2019

mayurkale22 commented Oct 16, 2019

feat: implement metrics sdk #415

feat: implement metrics sdk #415

Conversation

mayurkale22 commented Oct 9, 2019 • edited Loading

Which problem is this PR solving?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 9, 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 commented Oct 10, 2019

mayurkale22 commented Oct 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan commented Oct 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 commented Oct 16, 2019

mayurkale22 commented Oct 9, 2019 •

edited

Loading

codecov-io commented Oct 9, 2019 •

edited

Loading