Adds Metrics API #105

bg451 · 2019-07-16T01:21:26Z

Based on https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/metrics-api.md

Resolves #51

I feel like the metrics API can be a bit confusing so here's a code example of how it is used, modified from the example in the java repo. Here is an example for recording a raw measurement.

// getMeter is still undefined by spec
var meter: Meter = tracer.getMeter();
var cacheHit: Measure = meter.createMeasure("cache_hit", {
  description: "successful cache hit",
  type: MeasureType.LONG,
})
...
if (inCache(request)) {
  meter.record(cacheHit.createMeasurement(1))
}
...

And an example for using a metric

// getMeter is still undefined by spec
var meter: Meter = tracer.getMeter();
var requestCount: Metric<CounterTimeseries> = meter.createCounter("request_count", {
  description: "number of requests a customer has made",
  dynamicAttributes: ["customer_id"],
  component: "express",
})

// ...

func handleRequest(request) {
 // ...
  const customer_id: string = request.getCustomerID();
  requestCount.getOrCreateTimeseries(customer_id).add(1);
  // ...
}

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/metrics-api.md adds gauge and counter types checkpoint checkpoint update docs update index.ts move todo

draffensperger · 2019-07-16T12:52:19Z

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

+
+export interface CounterTimeseries {
+  // Adds the given value to the current value. Values cannot be negative.
+  add(value: number): void;


Maybe a topic for the SIG, but would it be worth putting a comment explaining that bigint is not currently supported but may be added in a future release?

I think it's a good topic, added to the sig agenda.

draffensperger · 2019-07-16T12:52:59Z

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

+
+export enum MeasureType {
+  DOUBLE = 0,
+  LONG = 1,


If we don't have a plan yet for the LONG case (since we just accept number as an input), should we just remove this and add a comment in its place?

draffensperger · 2019-07-16T12:54:27Z

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

+}
+
+// Metric represents a base class for different types of metric preaggregations.
+export interface Metric<T> {


Are there any bounds on what type T can be? And what does type T here mean?

Sadly it's not really spelled out in the specification but there's a somewhat hidden type called TimeSeries. So let's say you have a counter metric. In order to actually add something to the metric, you need to get the TimeSeries first by calling requestCount.getOrCreateTimeseries(customer_id). T represents the specific timeseries implementation. Pretty similar to what census had.

Right now Counter and Gauge have the same interface in the specification (although maybe they shouldn't). I'm curious to see what the histogram and summary types are going to look like.

edit: It's these interfaces here

I realize this is already merged, but getting caught up here. When I see a templated type like T, I tend to assume that as a user I can change that type.

But in this case it's really more like unknown - something opaque that just gets passed around. Why is the template needed on the Metric type? Could the implementation just be a private property or similar?

rochdev · 2019-07-16T15:13:38Z

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

+  add(value: number): void;
+
+  // Sets the given value. Value must be larger than the current recorded value.
+  set(value: number): void;


Should these be increment and decrement instead? It would make more sense for a counter.

Yeah probably. I'm also curious where the Values cannot be negative. requirement comes from.

I would think that fundamentally a "counter" represents counting something, which intuitively can't be negative. If a value can be negative, then to me it is not really a count of something tangible but rather represents something subtly different. For example, what does it mean that a server served -20 requests?

On the other hand, I could see something like "available CPU-seconds" being negative if the process is in debt to its scheduler. But that metric type I would say is a gauge and not a counter.

Side note, based on the rfcs it looks like counters only have add() and gauges only have set() now. There are options to allow negative options for counters or indicate that a gauge only increases in value.

OK, interesting - these are squishy concepts in some ways. Within the Stackdriver Monitoring product, counters (called "cumulative" in our API) have the interesting difference from gauges, in that you can take the rate of a cumulative but can't for a gauge.

rochdev · 2019-07-16T15:14:21Z

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

+
+export interface GaugeTimeseries {
+  // Adds the given value to the current value. Values can be negative.
+  add(value: number): void;


What is the purpose of add? A gauge by definition can only be set. If incrementing and decrementing is wanted, then a counter should probably used instead.

I'm not entirely sure, it looks to be pretty close to what census had.

OpenCensus has inc and reset for counters and add and inc for gauges.

Ah cool was looking at this, didn't see the impl for cumulative.

There's currently an RFC from @jmacd to change the verbiage for metrics, along with removing meter.record https://github.com/open-telemetry/rfcs/pull/4/files#diff-a4b53eafb1f61334abc351feb3ecc5b3R20

This is the impl for cumulative: https://github.com/census-instrumentation/opencensus-node/blob/master/packages/opencensus-core/src/metrics/cumulative/cumulative.ts#L196

I would agree that add feels a bit weird for gauges, but I could also imagine cases where you want it. I gave the example above of some resource budget that you can both deposit and withdraw from (and maybe even go negative with). That metric type I would consider a gauge because we allow it to be negative and it's more like measuring the amount of something available at a given time (like temperature) instead of counting occurrences of an event stream (like request count)

rochdev · 2019-07-16T15:20:29Z

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

+
+  // List of attribute keys with dynamic values. Order of list is important
+  // as the same order must be used when supplying values for these attributes.
+  dynamicAttributes?: string[];


Should this be a Map instead? Otherwise it will be difficult to update a value in the list.

jmacd · 2019-07-16T21:53:17Z

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

+  // Unit of the Metric values.
+  unit?: string;
+
+  // List of attribute keys with dynamic values. Order of list is important


As an aside, I feel that if we're passing Key:Value objects to the metrics / stats APIs, then we should be able to relax this statement about the order being important. IOW I interpret the dynamicAttributes as a recommended set of keys for aggregation: when it actually comes time to aggregate, it should not matter what order the attributes appear in a parameter list, whether they are defined in resources or in context tags.

mayurkale22 · 2019-09-11T19:37:34Z

Per SIG meeting (09/11), we planned to merge this initial API and revise it.

Set the new TracerProvider as delegate in ProxyTracerProvider only if global registration was successful.

Adds Metrics API

706d9d3

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/metrics-api.md adds gauge and counter types checkpoint checkpoint update docs update index.ts move todo

bg451 requested review from danielkhan, iredelmeier, justindsmith, mayurkale22 and rochdev as code owners July 16, 2019 01:21

yarn check

6bccf2a

draffensperger reviewed Jul 16, 2019

View reviewed changes

rochdev reviewed Jul 16, 2019

View reviewed changes

jmacd reviewed Jul 16, 2019

View reviewed changes

mayurkale22 added the onHold label Jul 27, 2019

bg451 mentioned this pull request Sep 5, 2019

Add Metrics API #51

Closed

Merge branch 'master' into add_metrics_api

f34b813

mayurkale22 requested a review from OlivierAlbertini as a code owner September 11, 2019 19:16

mayurkale22 approved these changes Sep 11, 2019

View reviewed changes

Merge branch 'master' into add_metrics_api

14aeb42

bg451 removed the onHold label Sep 12, 2019

mayurkale22 merged commit 50047b3 into open-telemetry:master Sep 12, 2019

mayurkale22 mentioned this pull request Sep 12, 2019

feat: Metrics API revision based on Specs and RFCs #259

Merged

mayurkale22 mentioned this pull request Oct 4, 2019

Implement Metrics SDK #402

Closed

4 tasks

dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022

fix: set delegate after successful registration (open-telemetry#105)

430de9b

Set the new TracerProvider as delegate in ProxyTracerProvider only if global registration was successful.

dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022

fix: set delegate after successful registration (open-telemetry#105)

4711f90

Set the new TracerProvider as delegate in ProxyTracerProvider only if global registration was successful.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Metrics API #105

Adds Metrics API #105

bg451 commented Jul 16, 2019

draffensperger Jul 16, 2019

bg451 Jul 16, 2019

draffensperger Jul 16, 2019

draffensperger Jul 16, 2019

bg451 Jul 16, 2019 •

edited

Loading

draffensperger Sep 13, 2019

rochdev Jul 16, 2019

bg451 Jul 16, 2019

draffensperger Sep 13, 2019

bg451 Sep 13, 2019

draffensperger Sep 13, 2019

rochdev Jul 16, 2019

bg451 Jul 16, 2019

mayurkale22 Jul 16, 2019

bg451 Jul 17, 2019

mayurkale22 Jul 17, 2019

draffensperger Sep 13, 2019

rochdev Jul 16, 2019

jmacd Jul 16, 2019

mayurkale22 commented Sep 11, 2019

Adds Metrics API #105

Adds Metrics API #105

Conversation

bg451 commented Jul 16, 2019

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

bg451 Jul 16, 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

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

Choose a reason for hiding this comment

mayurkale22 commented Sep 11, 2019

bg451 Jul 16, 2019 •

edited

Loading