Conversation
| self._len_label_keys = len(label_keys) | ||
| self.descriptor = metric_descriptor.MetricDescriptor( | ||
| name, description, unit, self.descriptor_type, label_keys) | ||
| self.points = OrderedDict() |
There was a problem hiding this comment.
This is so the points appear in the converted metric's timeseries list in the order that they're added, may not be necessary.
| self.value | ||
| )) | ||
|
|
||
| def add(self, val): |
There was a problem hiding this comment.
total fly by comment as I watch the metric progress: it strikes me as odd for a gauge to have an add method instead of just set and seems like it could lead to misuse by users, do you know the rationale behind this?
Is it because the value is threadsafe with the lock private, so a user couldn't do an atomic add without the functionality baked in?
There was a problem hiding this comment.
I think the motivation here is to support data sources that only expose a delta.
I'm following the java implementation for now, but we need to add a description of the gauges API (including add) to the spec.
Is it because the value is threadsafe with the lock private, so a user couldn't do an atomic add without the functionality baked in?
It's actually the other way around: we wouldn't have to lock in set if we didn't want to provide an atomic add. That said, there's nothing (besides common sense) preventing the user from acquiring the lock themselves and changing the value.
| converted metrics. See that class for details. | ||
| """ | ||
|
|
||
| def __init__(self, name, description, unit, label_keys): |
There was a problem hiding this comment.
I think we should validate all input params like name, label_keys etc.
There was a problem hiding this comment.
Just that they're not null or empty? We check label_keys in the MetricDescriptor constructor:
opencensus-python/opencensus/metrics/export/metric_descriptor.py
Lines 133 to 137 in 88ad971
opencensus/metrics/export/gauge.py
Outdated
| self.points | ||
| )) | ||
|
|
||
| def get_time_series(self, label_values): |
There was a problem hiding this comment.
I think is getDefaultTimeSeries missing. https://github.com/census-instrumentation/opencensus-java/blob/master/api/src/main/java/io/opencensus/metrics/DoubleGauge.java#L104
There was a problem hiding this comment.
That's true, I didn't understand the rationale for the default time series in the java client. The goal is that there's a single default point (read: time series) for each gauge that isn't associated with any label values?
There was a problem hiding this comment.
Added in a2e56cf, let me know what you think.
mayurkale22
left a comment
There was a problem hiding this comment.
Overall looks good to me.
This PR adds
LongGaugeandDoubleGaugemetrics classes following the java implementation, and incidentally adds/changes some related classes' repr methods to help inspect gauges.