-
Notifications
You must be signed in to change notification settings - Fork 792
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( |
||
|
||
// 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 | ||
|
@@ -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>; | ||
} |
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't believe so since this is different from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per the spec |
||
|
||
/** | ||
* The unit of the Metric values. | ||
* @default '1' | ||
*/ | ||
unit?: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the default value, based on Java's implementation: https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/metrics/Metric.java#L93 |
||
|
||
/** 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? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} |
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ofCumulative
There was a problem hiding this comment.
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