Add metric API #975
Add metric API #975
Conversation
|
The follow-ups are here: https://github.com/ramonza/opencensus-go/pulls |
|
@rakyll split up the PR, PTAL |
| func NewRegistry() *Registry { | ||
| m := &Registry{ | ||
| sources: make(map[*uintptr]Producer), | ||
| ind: 0, |
There was a problem hiding this comment.
Unused remainder from before the uintptr trick :)?
metric/doc.go
Outdated
|
|
||
| // Package metric contains a data model and exporter support for metrics. | ||
| // The data model is derived from the metric protocol buffers: | ||
| // https://github.com/census-instrumentation/opencensus-proto/blob/master/src/opencensus/proto/metrics/v1/metrics.proto |
There was a problem hiding this comment.
User doesn't have to know about the proto, please remove this.
|
|
||
| // LabelValue represents the value of a label. A missing value (nil) is distinct | ||
| // from an empty string value. | ||
| type LabelValue *string |
There was a problem hiding this comment.
This is a unlikely type. Why not simply type LabelValue { Value string }?
There was a problem hiding this comment.
The intention here is to be able to represent no value (nil) and empty string value distinctly. The protobuf does this, so we need to as well to be compatible. Using a type like you suggest would not allow us to represent these two distinctly.
There was a problem hiding this comment.
Protobuf types are not an idiomatic due to the limitations and code-generation nature of the problem. In handcrafted libraries, we are doing better than proto.
There was a problem hiding this comment.
We proposed to replace with this with struct { Value string, HasValue bool } to differentiate between "missing" and an empty string value.
| // from an empty string value. | ||
| type LabelValue *string | ||
|
|
||
| // NewLabelValue creates a new non-nil LabelValue that represents the given string. |
There was a problem hiding this comment.
This becomes if the type changes.
LabelValue{ Value: "val" } would just work.
metric/point.go
Outdated
| // [0, bucket_bounds[i]) for i == 0 | ||
| // [bucket_bounds[i-1], bucket_bounds[i]) for 0 < i < N-1 | ||
| // [bucket_bounds[i-1], +infinity) for i == N-1 | ||
| ExplicitBoundaries []float64 |
metric/point.go
Outdated
| ) | ||
|
|
||
| // NewDoublePoint creates a new Point holding a float64 value. | ||
| func NewDoublePoint(t time.Time, val float64) Point { |
| m.mu.Lock() | ||
| defer m.mu.Unlock() | ||
| if source == m { | ||
| panic("attempt to add registry to itself") |
There was a problem hiding this comment.
Why is this statically possible?
There was a problem hiding this comment.
*Registry implements Producer
There was a problem hiding this comment.
The question is it might be broken to make Registry implementing Read() []*Metric.
Can't we make it implement ReadAll() []*Metric to differentiate it from the producers? Otherwise, we should introduce a MultiProducer a la io.MultiReader instead of this type.
metric/metric.go
Outdated
| // Metric represents a quantity measured against a resource with different | ||
| // label value combinations. | ||
| type Metric struct { | ||
| Descriptor *Descriptor // metric descriptor |
There was a problem hiding this comment.
Descriptor Descriptor
No need for a pointer.
| // value of this point. | ||
| // Consumers of Point should use this in preference to switching on the type | ||
| // of the value directly, since new value types may be added. | ||
| func (p Point) ReadValue(vv ValueVisitor) { |
There was a problem hiding this comment.
Is this something that exporters would use? If yes, maybe we shouldn't have it in this package.
There was a problem hiding this comment.
Yes, it is. This whole package is mostly for use by exporters.
There was a problem hiding this comment.
Is there any end to end example usage for the new APIs here? It is not clear what the package is for. If it is mainly for the exporters, we shouldn't use a top-level package like "metric" where we would like to put the developer-facing APIs to.
| @@ -0,0 +1,11 @@ | |||
| package metric | |||
| @@ -0,0 +1,29 @@ | |||
| package metric | |||
|
LGTM |
This reverts commit a91cf18. The API was merged without an API review.
|
|
||
| // LabelValue represents the value of a label. A missing value (nil) is distinct | ||
| // from an empty string value. | ||
| type LabelValue *string |
There was a problem hiding this comment.
Protobuf types are not an idiomatic due to the limitations and code-generation nature of the problem. In handcrafted libraries, we are doing better than proto.
| m.mu.Lock() | ||
| defer m.mu.Unlock() | ||
| if source == m { | ||
| panic("attempt to add registry to itself") |
There was a problem hiding this comment.
The question is it might be broken to make Registry implementing Read() []*Metric.
Can't we make it implement ReadAll() []*Metric to differentiate it from the producers? Otherwise, we should introduce a MultiProducer a la io.MultiReader instead of this type.
| // rely on the DefaultRegistry. | ||
| type Registry struct { | ||
| mu sync.RWMutex | ||
| sources map[*uintptr]Producer |
There was a problem hiding this comment.
In what case? Also why a pointer to uintptr?
| // value of this point. | ||
| // Consumers of Point should use this in preference to switching on the type | ||
| // of the value directly, since new value types may be added. | ||
| func (p Point) ReadValue(vv ValueVisitor) { |
There was a problem hiding this comment.
Is there any end to end example usage for the new APIs here? It is not clear what the package is for. If it is mainly for the exporters, we shouldn't use a top-level package like "metric" where we would like to put the developer-facing APIs to.
No description provided.