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

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

Merged
merged 2 commits into from
Sep 13, 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
9 changes: 4 additions & 5 deletions packages/opentelemetry-types/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,10 @@ 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 './metrics/Handle';
export * from './metrics/Measure';
export * from './metrics/Meter';
export * from './metrics/Metric';
export * from './resources/Resource';
export * from './trace/attributes';
export * from './trace/Event';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,32 @@
* limitations under the License.
*/

export interface CounterTimeseries {
// Adds the given value to the current value. Values cannot be negative.
/** A Handle for Counter Metric. */
export interface CounterHandle {
Copy link
Member

Choose a reason for hiding this comment

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

Rename to counter to cumulative

Copy link
Member Author

Choose a reason for hiding this comment

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

Btw, I thought we have decided to go with Counter instead of Cumulative

Copy link
Member

Choose a reason for hiding this comment

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

cool beans just saw the PR in the rfc section to change it back to counter

/**
* Adds the given value to the current value. Values cannot be negative.
* @param value the value to add
*/
add(value: number): void;

// Sets the given value. Value must be larger than the current recorded value.
/**
* Sets the given value. Value must be larger than the current recorded value.
* @param value the new value.
*/
set(value: number): void;
}

/** A Handle for Gauge Metric. */
export interface GaugeHandle {
/**
* Adds the given value to the current value. Values can be negative.
* @param value the value to add.
*/
add(value: number): void;

/**
* Sets the given value. Values can be negative.
* @param value the new value.
*/
set(value: number): void;
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,17 @@
* limitations under the License.
*/

import { DistributedContext } from '../distributed_context/DistributedContext';
import { SpanContext } from '../trace/span_context';

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

/**
* Options needed for measure creation
*/
export interface MeasureOptions {
// Description of the Measure.
description?: string;
Expand All @@ -30,10 +36,20 @@ export interface MeasureOptions {
type?: MeasureType;
}

/** Measure to report instantaneous measurement of a value. */
export interface Measure {
// Creates a measurement with the supplied value.
createMeasurement(value: number): Measurement;
/**
* Records the given value to this measure.
* @param value the measurement to record.
* @param distContext the distContext associated with the measurements.
* @param spanContext the {@link SpanContext} that identifies the {@link Span}
* for which the measurements are associated with.
*/
record(value: number): void;
record(value: number, distContext: DistributedContext): void;
record(
value: number,
distContext: DistributedContext,
spanContext: SpanContext
): void;
}

// Measurement describes an individual measurement
export interface Measurement {}
Original file line number Diff line number Diff line change
Expand Up @@ -14,30 +14,31 @@
* 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;
}
import { Metric, MetricOptions } from './Metric';
import { CounterHandle, GaugeHandle } from './Handle';
import { MeasureOptions, Measure } from './Measure';

/**
* An interface to allow the recording metrics.
*
* {@link Metric}s are used for recording pre-defined aggregation (Gauge and
* Counter), or raw values ({@link Measure}) in which the aggregation and labels
* for the exported metric are deferred.
*/
export interface Meter {
// Creates and returns a new @link{Measure}.
/**
* Creates and returns a new {@link Measure}.
* @param name the name of the metric.
* @param [options] the measure options.
*/
createMeasure(name: string, options?: MeasureOptions): Measure;
Copy link
Member

Choose a reason for hiding this comment

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

I took a stab at updates the other day, take a look at bg451#5. I think this should return Metric<MeasureHandle>, though I can apply the changes in a separate PR

Copy link
Member Author

@mayurkale22 mayurkale22 Sep 13, 2019

Choose a reason for hiding this comment

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

bg451#5 lgtm, I will merge this one and wait for your PR for the next revision of metrics API (including counter -> cumulative).


// Creates a new counter metric.
createCounter(
name: string,
options?: MetricOptions
): Metric<CounterTimeseries>;
/**
* Creates a new counter metric.
* @param name the name of the metric.
* @param [options] the metric options.
*/
createCounter(name: string, options?: MetricOptions): Metric<CounterHandle>;

// TODO: Measurements can have a long or double type. However, it looks like
// the metric timeseries API (according to spec) accepts values instead of
Expand All @@ -47,9 +48,10 @@ export interface Meter {
// 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;
/**
* Creates a new gauge metric.
* @param name the name of the metric.
* @param [options] the metric options.
*/
createGauge(name: string, options?: MetricOptions): Metric<GaugeHandle>;
}
81 changes: 81 additions & 0 deletions packages/opentelemetry-types/src/metrics/Metric.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/**
* 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 { Resource } from '../resources/Resource';

/**
* Options needed for metric creation
*/
export interface MetricOptions {
/**
* The description of the Metric.
* @default ''
*/
description?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be required?

Copy link
Member

Choose a reason for hiding this comment

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

I don't believe so since this is different from name

Copy link
Member Author

Choose a reason for hiding this comment

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

As per the spec description is an optional param and the default to "".


/**
* The unit of the Metric values.
* @default '1'
*/
unit?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we specify the implied default unit if this is not specified? E.g. "count"? And should we specify canonical units for things like bytes, seconds, bytes/second, count/second, etc.?

Is there a formal spec for this? Should we adopt something like http://metrics20.org/spec/#tag-values-unit ?

Copy link
Member Author

Choose a reason for hiding this comment

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


/** The list of label keys for the Metric. */
labelKeys?: string[];

/** The map of constant labels for the Metric. */
constantLabels?: Map<string, string>;

/** The resource the Metric is associated with. */
resource?: Resource;

/** The name of the component that reports the Metric. */
component?: string;
}

/**
* Metric represents a base class for different types of metric
* pre aggregations.
*/
export interface Metric<T> {
/**
* Returns a Handle associated with specified label values.
* It is recommended to keep a reference to the Handle instead of always
* calling this method for every operations.
* @param labelValues the list of label values.
*/
getHandle(labelValues: string[]): T;

/**
* Returns a Handle for a metric with all labels not set.
*/
getDefaultHandle(): T;

/**
* Removes the Handle from the metric, if it is present.
* @param labelValues the list of label values.
*/
removeHandle(labelValues: string[]): void;

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

/**
* what should the callback signature be?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the signature is fine personally. But could you document what the callback does and when it gets called?

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 am not 100% sure about this, @bg451 could you please handle this in your next PR?

*/
setCallback(fn: () => void): void;
}
22 changes: 0 additions & 22 deletions packages/opentelemetry-types/src/metrics/gauge.ts

This file was deleted.

63 changes: 0 additions & 63 deletions packages/opentelemetry-types/src/metrics/metric.ts

This file was deleted.