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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/opentelemetry-types/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ export * from './context/propagation/BinaryFormat';
export * from './context/propagation/HttpTextFormat';
export * from './distributed_context/DistributedContext';
export * from './distributed_context/EntryValue';
export * from './metrics/counter';
export * from './metrics/gauge';
export * from './metrics/measure';
export * from './metrics/meter';
export * from './metrics/metrics';
export * from './resources/Resource';
export * from './trace/attributes';
export * from './trace/Event';
Expand Down
23 changes: 23 additions & 0 deletions packages/opentelemetry-types/src/metrics/counter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/**
* Copyright 2019, OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

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.


// 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.

}
22 changes: 22 additions & 0 deletions packages/opentelemetry-types/src/metrics/gauge.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* Copyright 2019, OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

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)

// Sets the given value. Values can be negative.
set(value: number): void;
}
39 changes: 39 additions & 0 deletions packages/opentelemetry-types/src/metrics/measure.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/**
* Copyright 2019, OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

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?

}

export interface MeasureOptions {
// Description of the Measure.
description?: string;

// Unit of the Measure.
unit?: string;

// Type of the Measure. Default type is DOUBLE.
type?: MeasureType;
}

export interface Measure {
// Creates a measurement with the supplied value.
createMeasurement(value: number): Measurement;
}

// Measurement describes an individual measurement
export interface Measurement {}
55 changes: 55 additions & 0 deletions packages/opentelemetry-types/src/metrics/meter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/**
* Copyright 2019, OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { SpanContext } from '../trace/span_context';
import { DistributedContext } from '../distributed_context/DistributedContext';
import { Measure, MeasureOptions, Measurement } from './measure';
import { Metric, MetricOptions } from './metric';
import { CounterTimeseries } from './counter';
import { GaugeTimeseries } from './gauge';

export interface RecordOptions {
// spanContext represents a measurement exemplar in the form of a SpanContext.
spanContext?: SpanContext;
// distributedContext overrides the current context and adds dimensions
// to the measurements.
distributedContext?: DistributedContext;
}

export interface Meter {
// Creates and returns a new @link{Measure}.
createMeasure(name: string, options?: MeasureOptions): Measure;

// Creates a new counter metric.
createCounter(
name: string,
options?: MetricOptions
): Metric<CounterTimeseries>;

// TODO: Measurements can have a long or double type. However, it looks like
// the metric timeseries API (according to spec) accepts values instead of
// Measurements, meaning that if you accept a `number`, the type gets lost.
// Both java and csharp have gone down the route of having two gauge interfaces,
// GaugeDoubleTimeseries and GaugeLongTimeseries, with param for that type. It'd
// be cool to only have a single interface, but maybe having two is necessary?
// Maybe include the type as a metrics option? Probs a good gh issue, the same goes for Measure types.

// Creates a new gauge metric.
createGauge(name: string, options?: MetricOptions): Metric<GaugeTimeseries>;

// Record a set of raw measurements.
record(measurements: Measurement[], options?: RecordOptions): void;
}
63 changes: 63 additions & 0 deletions packages/opentelemetry-types/src/metrics/metric.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/**
* Copyright 2019, OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { Attributes } from '../trace/attributes';
import { Resource } from '../resources/Resource';

export interface MetricOptions {
// Description of the Metric.
description?: string;

// 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.

// 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.


// List of attributes with constant values.
constantAttributes?: Attributes;

// Resource the metric is associated with.
resource?: Resource;

// Name of the component that reports the metric.
component?: string;
}

// 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?

// Creates a timeseries if the specified attribute values
// are not associated with an existing timeseries, otherwise returns the
// existing timeseries.
// Order and number of attribute values MUST match the order and number of
// dynanic attribute keys when the Metric was created.
getOrCreateTimeseries(values: unknown[]): T;

// Returns a timeseries with all attribute values not set.
getDefaultTimeseries(): T;

// Removes an existing timeseries. Order and number of attribute values MUST
// match the order and number of dynamic attribute keys when the Metric was
// created.
removesTimeseries(values: unknown[]): void;

// Clears all timeseries from the Metric.
clear(): void;

// todo: what should the callback signature be?
setCallback(fn: () => void): void;
}