Skip to content
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

Add MetricDescriptor class, tests #342

Merged
merged 2 commits into from
Oct 10, 2018

Conversation

c24t
Copy link
Member

@c24t c24t commented Oct 9, 2018

This is a largely mechanical translation of the Java client's MetricDescriptor class, in the style of the other metrics classes from #337.

Addresses #335.

MetricDescriptorType is an enum of valid MetricDescriptor type values. See
opencensus-proto for details:

https://github.com/census-instrumentation/opencensus-proto/blob/24333298e36590ea0716598caacc8959fc393c48/src/opencensus/proto/metrics/v1/metrics.proto#L73 # noqa
Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure whether to include this, but since we're copying these wholesale from the spec I thought it'd be helpful to have a link. We might also consider generating code for this kind of thing.

and sets a new start time for the following points.

"""
__metaclass__ = _MetricDescriptorTypeMeta
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to change this if you've got a different preferred way of handling enums.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to use six.add_metaclass for python 3.

@c24t c24t force-pushed the add-metrics-descriptor branch from fd9e4f9 to 99b510b Compare October 9, 2018 21:09
@c24t c24t force-pushed the add-metrics-descriptor branch from 99b510b to 12057a4 Compare October 9, 2018 21:35
Copy link
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

LGTM

@songy23
Copy link
Contributor

songy23 commented Oct 10, 2018

@c24t Do you have access to merge commits now?

@c24t
Copy link
Member Author

c24t commented Oct 10, 2018

@c24t Do you have access to merge commits now?

I do, thanks!

@c24t c24t merged commit 9c07ae4 into census-instrumentation:master Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants