Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds Metrics API #105

Merged
merged 4 commits into from
Sep 12, 2019
Merged

Adds Metrics API #105

merged 4 commits into from
Sep 12, 2019

Conversation

bg451
Copy link
Member

@bg451 bg451 commented Jul 16, 2019

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

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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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


export enum MeasureType {
DOUBLE = 0,
LONG = 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

}

// Metric represents a base class for different types of metric preaggregations.
export interface Metric<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@bg451 bg451 Jul 16, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

add(value: number): void;

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)


// 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[];
Copy link
Member

Choose a reason for hiding this comment

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

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

// Unit of the Metric values.
unit?: string;

// List of attribute keys with dynamic values. Order of list is important
Copy link

Choose a reason for hiding this comment

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

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.

@bg451 bg451 mentioned this pull request Sep 5, 2019
@mayurkale22
Copy link
Member

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

@bg451 bg451 removed the onHold label Sep 12, 2019
@mayurkale22 mayurkale22 merged commit 50047b3 into open-telemetry:master Sep 12, 2019
@mayurkale22 mayurkale22 mentioned this pull request Oct 4, 2019
4 tasks
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
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
Set the new TracerProvider as delegate in ProxyTracerProvider
only if global registration was successful.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Metrics API
5 participants