feat: Metrics API revision based on Specs and RFCs #259
Conversation
mayurkale22
requested review from
bg451,
danielkhan,
iredelmeier,
OlivierAlbertini and
rochdev
as code owners
mayurkale22
force-pushed
the
metrics_v2
branch
2 times, most recently
from
c4b2d10
to
c51b375
Compare
mayurkale22
force-pushed
the
metrics_v2
branch
from
c51b375
to
134e986
Compare
| */ | ||
| export interface MetricOptions { | ||
| /** The description of the Metric. */ | ||
| description?: string; |
There was a problem hiding this comment.
Should this be required?
There was a problem hiding this comment.
I don't believe so since this is different from name
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
Btw, I thought we have decided to go with Counter instead of Cumulative
There was a problem hiding this comment.
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.
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
TimeSeriesbecomesHandlein RFC 0003, 0007 updates (Metrics measure type, metrics handle specification), RFC 0004 (configurable aggregation) deleted oteps#29.Added/Corrected JSDoc comments.
Renamed
constantAttributestoconstantLabelsas per the specs.dynamicAttributeschanged tolabelKeysas 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).