Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

Add Metric and supporting classes#347

Merged
c24t merged 10 commits intocensus-instrumentation:masterfrom
c24t:add-metric
Oct 16, 2018
Merged

Add Metric and supporting classes#347
c24t merged 10 commits intocensus-instrumentation:masterfrom
c24t:add-metric

Conversation

@c24t
Copy link
Copy Markdown
Member

@c24t c24t commented Oct 12, 2018

Like #342, this diff adds a few metrics classes following the proto spec and java implementation. The new classes are Metric, TimeSeries, and the value class ValueDistribution.

This diff is written in the style of #337, following the java package structure. I included preconditions to match the comments in the metrics proto file, but this client's behavior isn't guaranteed to match the java client's.

Addresses #335.

Comment thread opencensus/metrics/export/metric.py Outdated
Comment thread opencensus/metrics/export/metric.py Outdated
Comment thread opencensus/metrics/export/metric.py Outdated
Comment thread opencensus/metrics/export/metric.py
Comment thread opencensus/metrics/export/time_series.py Outdated
Comment thread opencensus/metrics/export/time_series.py Outdated
Comment thread opencensus/metrics/export/time_series.py Outdated
:param value: Value of the exemplar point, determines which bucket the
exemplar belongs to.

:type timestamp: str
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type time

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, unless time has some special meaning I'm missing here.

Comment thread opencensus/metrics/export/value.py
c24t added a commit to c24t/opencensus-python that referenced this pull request Oct 13, 2018
c24t and others added 2 commits October 15, 2018 11:42
and add a check for cumulative metrics with missing start timestamps.
Copy link
Copy Markdown
Contributor

@liyanhui1228 liyanhui1228 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor comments.

Comment thread opencensus/metrics/export/metric.py Outdated

Defines a Metric which has one or more timeseries.

:type descriptor: class: '~opencensus.metrics.export.metric_descriptor.MetricDescriptor' # noqa
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why adding # noqa here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Force of habit for long lines, I moved this and another noqa to the end of the docstring in 4a47970.

metric_descriptor.MetricDescriptorType.SUMMARY):
check_type = value.ValueSummary
else:
raise ValueError("Unknown metric descriptor type")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# This relation mapping could be put at the top of this file
DESCRIPTOR_VALUE = {
    metric_descriptor.MetricDescriptorType.GAUGE_INT64: value.ValueLong,
    metric_descriptor.MetricDescriptorType.CUMULATIVE_INT64: value.ValueLong,
    ...
}

check_type = DESCRIPTOR_VALUE.get(self.descriptor.type)
if check_type is None:
    raise ValueError("Unknown metric descriptor type")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, changed in 39d2dcb.

@c24t c24t self-assigned this Oct 16, 2018
@c24t c24t merged commit e72a4dc into census-instrumentation:master Oct 16, 2018
@c24t c24t deleted the add-metric branch October 16, 2018 18:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants