-
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
Conversation
c4b2d10
to
c51b375
Compare
c51b375
to
134e986
Compare
*/ | ||
export interface MetricOptions { | ||
/** The description of the Metric. */ | ||
description?: string; |
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.
Should this be required?
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.
I don't believe so since this is different from name
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.
As per the spec description
is an optional param and the default to "".
description?: string; | ||
|
||
/** The unit of the Metric values. */ | ||
unit?: string; |
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.
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 comment
The 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
clear(): void; | ||
|
||
/** | ||
* what should the callback signature be? |
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.
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 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?
* 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 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
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.
bg451#5 lgtm, I will merge this one and wait for your PR for the next revision of metrics API (including counter -> cumulative).
export interface CounterTimeseries { | ||
// Adds the given value to the current value. Values cannot be negative. | ||
/** A Handle for Counter Metric. */ | ||
export interface CounterHandle { |
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 of 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.
cool beans just saw the PR in the rfc section to change it back to counter
*/ | ||
export interface MetricOptions { | ||
/** The description of the Metric. */ | ||
description?: string; |
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.
I don't believe so since this is different from name
…ore using it (open-telemetry#259) * fix(plugin-document-load): check if getEntriesByType is available before using it * fix: lint Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Which problem is this PR solving?
Continuation of Adds Metrics API #105
Metrics API revision based on specs(https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics.md) and RFCs (RFC 0003, 0007 updates (Metrics measure type, metrics handle specification), RFC 0004 (configurable aggregation) deleted oteps#29)
Short description of the changes
TimeSeries
becomesHandle
in RFC 0003, 0007 updates (Metrics measure type, metrics handle specification), RFC 0004 (configurable aggregation) deleted oteps#29.Added/Corrected JSDoc comments.
Renamed
constantAttributes
toconstantLabels
as per the specs.dynamicAttributes
changed tolabelKeys
as per the specs (list of keys for the labels with dynamic values. The order of the list is important as the same order MUST be used in recording when supplying values for these labels).