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

Metrics API with RFC 0003 #87

Merged
merged 35 commits into from
Sep 12, 2019
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
6ca4274
Create functions
lzchen Jul 29, 2019
b23cec1
fix lint
lzchen Jul 31, 2019
981eece
Fix lint
lzchen Jul 31, 2019
8ea9709
fix typing
lzchen Jul 31, 2019
00b4f11
Remove options, constructors, seperate labels
lzchen Aug 6, 2019
34c87ce
Consistent naming for float and int
lzchen Aug 6, 2019
df8ae34
Abstract time series
lzchen Aug 6, 2019
a2561ac
Use ABC
lzchen Aug 6, 2019
1ece493
Fix typo
lzchen Aug 6, 2019
ce9268a
Fix docs
lzchen Aug 6, 2019
f5f9f01
seperate measure classes
lzchen Aug 8, 2019
74a1815
Add examples
lzchen Aug 8, 2019
0a0b8ee
fix lint
lzchen Aug 8, 2019
555bf50
Update to RFC 0003
lzchen Aug 14, 2019
d6b1113
Add spancontext, measurebatch
lzchen Aug 14, 2019
c819109
Merge branch 'master' of https://github.com/open-telemetry/openteleme…
lzchen Aug 15, 2019
18cfc71
Fix docs
lzchen Aug 15, 2019
f646555
Merge branch 'master' of https://github.com/open-telemetry/openteleme…
lzchen Aug 19, 2019
a44cb47
Fix comments
lzchen Aug 22, 2019
94eaad9
fix lint
lzchen Aug 22, 2019
fc251b2
fix lint
lzchen Aug 22, 2019
262f312
fix lint
lzchen Aug 22, 2019
66c0a56
skip examples
lzchen Aug 22, 2019
e2c4a7e
white space
lzchen Aug 22, 2019
2fb7646
fix spacing
lzchen Aug 22, 2019
eb711cb
fix imports
lzchen Aug 22, 2019
baa3a32
fix imports
lzchen Aug 22, 2019
5c30a9c
Merge branch 'master' of https://github.com/open-telemetry/openteleme…
lzchen Sep 3, 2019
211b20c
LabelValues to str
lzchen Sep 3, 2019
bffe040
Black formatting
lzchen Sep 3, 2019
0759b9a
fix isort
lzchen Sep 3, 2019
44d62f8
Remove aggregation
lzchen Sep 12, 2019
c5ab2df
Fix names
lzchen Sep 12, 2019
50d2de5
Remove aggregation from docs
lzchen Sep 12, 2019
d79bc7d
Fix lint
lzchen Sep 12, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ abstract types for OpenTelemetry implementations.

opentelemetry.context
opentelemetry.loader
opentelemetry.metrics
opentelemetry.trace


Expand Down
5 changes: 5 additions & 0 deletions docs/opentelemetry.metrics.aggregation.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
opentelemetry.metrics.aggregation module
==========================================

.. automodule:: opentelemetry.metrics.aggregation

15 changes: 15 additions & 0 deletions docs/opentelemetry.metrics.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
opentelemetry.metrics package
=============================

Submodules
----------

.. toctree::

opentelemetry.metrics.aggregation
opentelemetry.metrics.time_series

Module contents
---------------

.. automodule:: opentelemetry.metrics
5 changes: 5 additions & 0 deletions docs/opentelemetry.metrics.time_series.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
opentelemetry.metrics.time\_series module
==========================================

.. automodule:: opentelemetry.metrics.time_series

331 changes: 331 additions & 0 deletions opentelemetry-api/src/opentelemetry/metrics/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,334 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

"""
The OpenTelemetry metrics API describes the classes used to report raw
measurements, as well as metrics with known aggregation and labels.

The `Meter` class is used to construct `Metric` s to record raw statistics
as well as metrics with predefined aggregation.

See the `metrics api`_ spec for terminology and context clarification.

.. _metrics api:
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics.md


"""

from abc import ABC
from abc import abstractmethod
from typing import List
from typing import Union

from opentelemetry.metrics.aggregation import Aggregation
from opentelemetry.metrics.time_series import CounterTimeSeries
from opentelemetry.metrics.time_series import GaugeTimeSeries
from opentelemetry.metrics.time_series import MeasureTimeSeries
from opentelemetry.trace import SpanContext

LabelKeys = List['LabelKey']
LabelValues = List['LabelValue']


class Meter:
"""An interface to allow the recording of metrics.

`Metric` s are used for recording pre-defined aggregation (gauge and
counter), or raw values (measure) in which the aggregation and labels
for the exported metric are deferred.
"""

def create_float_counter(self,
Copy link
Member

Choose a reason for hiding this comment

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

thoughts on passing in the float or int as types to the create method?

Might also be a good use case for enums:

def create(self, metric_type=METRIC_TYPE.counter, data_type=float, ...)

name: str,
description: str,
unit: str,
label_keys: LabelKeys,
span_context: SpanContext = None
) -> 'CounterFloat':
"""Creates a counter type metric that contains float values.

Args:
name: The name of the counter.
description: Human readable description of the metric.
unit: Unit of the metric values.
label_keys: list of keys for the labels with dynamic values.
Order of the list is important as the same order MUST be used
on recording when suppling values for these labels.
span_context: The `SpanContext` that identifies the `Span`
that the metric is associated with.

Returns: A new `CounterFloat`
"""

def create_int_counter(self,
name: str,
description: str,
unit: str,
label_keys: LabelKeys,
span_context: SpanContext = None
) -> 'CounterInt':
"""Creates a counter type metric that contains int values.

Args:
name: The name of the counter.
description: Human readable description of the metric.
unit: Unit of the metric values.
label_keys: list of keys for the labels with dynamic values.
Order of the list is important as the same order MUST be used
on recording when suppling values for these labels.
span_context: The `SpanContext` that identifies the `Span`
that the metric is associated with.

Returns:
A new `CounterInt`
"""

def create_float_gauge(self,
name: str,
description: str,
unit: str,
label_keys: LabelKeys,
span_context: SpanContext = None
) -> 'GaugeFloat':
"""Creates a gauge type metric that contains float values.

Args:
name: The name of the counter.
description: Human readable description of the metric.
unit: Unit of the metric values.
label_keys: list of keys for the labels with dynamic values.
Order of the list is important as the same order MUST be used
on recording when suppling values for these labels.
span_context: The `SpanContext` that identifies the `Span`
that the metric is associated with.

Returns:
A new `GaugeFloat`
Copy link
Member

Choose a reason for hiding this comment

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

Minor personal opinion here, it might be better to use create_float_guage with FloatGauge rather than mixing "float gauge" and "gauge float". Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I think I will leave this for the next PR, seeing as we are getting rid of TimeSeries and these naming changes should be done all at once for consistency.

Copy link

Choose a reason for hiding this comment

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

In the current metrics RFC PR open-telemetry/oteps#29, I've renamed TimeSeries to Handle following a consensus reached in the 8/21 working session, but it's just a name change relative to the code here.

"""

def create_int_gauge(self,
name: str,
description: str,
unit: str,
label_keys: LabelKeys,
span_context: SpanContext = None
) -> 'GaugeInt':
"""Creates a gauge type metric that contains int values.

Args:
name: The name of the counter.
description: Human readable description of the metric.
unit: Unit of the metric values.
label_keys: list of keys for the labels with dynamic values.
Order of the list is important as the same order MUST be used
on recording when suppling values for these labels.
span_context: The `SpanContext` that identifies the `Span`
that the metric is associated with.

Returns:
A new `GaugeInt`
"""

def create_int_measure(self,
name: str,
description: str,
unit: str,
label_keys: LabelKeys,
aggregation: 'Aggregation',
span_context: SpanContext = None,
) -> 'MeasureInt':
"""Creates a measure used to record raw int values.

Args:
name: The name of the measure.
description: Human readable description of this measure.
unit: Unit of the measure values.
label_keys: list of keys for the labels with dynamic values.
Order of the list is important as the same order MUST be used
on recording when suppling values for these labels.
aggregation: The type of aggregation to use for this metric.
span_context: The `SpanContext` that identifies the `Span`
that the metric is associated with.

Returns:
A new `MeasureInt`
"""

def create_float_measure(self,
name: str,
description: str,
unit: str,
label_keys: LabelKeys,
aggregation: 'Aggregation',
span_context: SpanContext = None,
) -> 'MeasureFloat':
"""Creates a Measure used to record raw float values.

Args:
name: the name of the measure
description: Human readable description of this measure.
unit: Unit of the measure values.
label_keys: list of keys for the labels with dynamic values.
Order of the list is important as the same order MUST be used
on recording when suppling values for these labels.
aggregation: The type of aggregation to use for this metric.
span_context: The `SpanContext` that identifies the `Span`
that the metric is associated with.

Returns:
A new `MeasureFloat`
"""


class Metric(ABC):
"""Base class for various types of metrics.

Metric class that inherit from this class are specialized with the type of
time series that the metric holds. Metric is constructed from the meter.
"""

@abstractmethod
def get_or_create_time_series(self,
Copy link
Member

Choose a reason for hiding this comment

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

this is probably for the RFC or OTel spec, but is there a better description than timeseries? I think most metric systems don't expose a nuance between a timeseries and an individual logging of a measure.

Copy link
Member

Choose a reason for hiding this comment

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

I think the best explanation for the name "timeseries" is that something got lost in the translation from OC code to the OT spec.

  1. TimeSeries used to be a list of a Metric's timestamped measurements.
  2. Gauges and counters aren't meant to record multiple measurements per export interval, just report the current value (for each set of labels) at export time. To be consistent with Metrics, they could export each value as a single-point TimeSeries.
  3. Fast-forward to the OT spec: we've kept the name, but changed the underlying logic. TimeSeries is now a container for single values, and Gauges are now Metrics.

I may be missing something here, but I don't see a reason that these should still be called "time series".

label_values: LabelValues
) -> 'object':
"""Gets and returns a timeseries, a container for a cumulative value.

If the provided label values are not already associated with this
metric, a new timeseries is returned, otherwise it returns the existing
timeseries with the exact label values. The timeseries returned
contains logic and behaviour specific to the type of metric that
overrides this function.

Args:
label_values: A map of `LabelValue` s that will be associated
with the return timeseries.
"""

def remove_time_series(self,
Copy link

Choose a reason for hiding this comment

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

I wonder if the remove method should take a Timeseries object, instead of a LabelSet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no problem with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might revisit this, if we decide to remove TimeSeries entirely.

Copy link
Member

Choose a reason for hiding this comment

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

If we treat Metric as a registry for these value containers (TimeSeries right now, but 👍 to changing this name or axing the class completely) then I don't think we should expect to get the same instance for the same label values at different times.

Another argument for keeping this signature: We expect users to call this to stop reporting a metric for a given set of label values. If they've got the time series they can use its label values without making another call to get_or_create_timeseries here.

label_values: LabelValues) -> None:
"""Removes the timeseries from the Metric, if present.

The timeseries with matching `LabelValue` s will be removed.

args:
label_values: The list of label values to match against.
"""

def clear(self) -> None:
"""Removes all timeseries from the `Metric`."""


class CounterFloat(Metric):
"""A counter type metric that holds float values.

Cumulative values can go up or stay the same, but can never go down.
Cumulative values cannot be negative.
"""

def get_or_create_time_series(self,
label_values: LabelValues
) -> 'CounterTimeSeries':
"""Gets a `CounterTimeSeries` with a cumulative float value."""


class CounterInt(Metric):
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity: how will these APIs be used in practice? I kind of imagine that we wouldn't hook in different implementations of a counter or gauge: the behavior feels pretty clear and might be more beneficial to remain consistent.

Again an example that makes opentelemetry-sdk basically a must to use opentelemetry-api

Copy link
Member

Choose a reason for hiding this comment

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

We see this problem with almost every class in the metrics package. Tracing has separate API and SDK packages largely because vendors want to be able to change the implementation. Metrics has separate API and SDK packages to be consistent with tracing, but if (e.g.) vendors are going to change metrics, they're much more likely to add new aggregations or metric types than to change these built-in classes.

This touches on a deeper issue regarding the API/SDK separation. The API already includes some implementation to support context propagation without an SDK, so how do we decide where to draw the line?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a great question to discuss in the specification. I agree with all the points you've raised here.

Copy link

Choose a reason for hiding this comment

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

The current metrics RFC discusses the semantic connection between metrics APIs and trace context. The current specification describes three kinds of metric "instrument", cumulative, gauge, and measure, which are pure API objects. Handles (known as TimeSeries currently) are SDK-specific implementations of the Add(), Set(), and Record() APIs. Nothing is said about aggregations at the API level apart from the the standard interpretation given to each, which is to say that by default cumulative metric instruments aggregate a sum, gauge metric instruments aggregate a last-value, and measure instruments aggregate a rate of some distribution.
open-telemetry/oteps#29

By separating the API from the implementation, it should be possible for to define an exporter for direct integration with existing metrics client libraries. This will require some sort of new metrics event exporter to be specified.

"""A counter type metric that holds int values.

Cumulative values can go up or stay the same, but can never go down.
Cumulative values cannot be negative.
"""

def get_or_create_time_series(self,
label_values: LabelValues
) -> 'CounterTimeSeries':
"""Gets a `CounterTimeSeries` with a cumulative int value."""


class GaugeFloat(Metric):
"""A gauge type metric that holds float values.

Cumulative value can go both up and down. Values can be negative.
"""

def get_or_create_time_series(self,
label_values: LabelValues
) -> 'GaugeTimeSeries':
"""Gets a `GaugeTimeSeries` with a cumulative float value."""


class GaugeInt(Metric):
"""A gauge type metric that holds int values.

Cumulative value can go both up and down. Values can be negative.
"""

def get_or_create_time_series(self,
label_values: LabelValues
) -> 'GaugeTimeSeries':
"""Gets a `GaugeTimeSeries` with a cumulative int value."""


class MeasureFloat(Metric):
"""A measure type metric that holds float values.

Measure metrics represent raw statistics that are recorded.
"""

def get_or_create_time_series(self,
label_values: LabelValues
) -> 'MeasureTimeSeries':
"""Gets a `MeasureTimeSeries` with a cumulated float value."""


class MeasureInt(Metric):
"""A measure type metric that holds int values.

Measure metrics represent raw statistics that are recorded.
"""

def get_or_create_time_series(self,
label_values: LabelValues
) -> 'MeasureTimeSeries':
"""Gets a `MeasureTimeSeries` with a cumulated int value."""


class MeasureBatch:

def record(self,
metrics: List['Metric'],
values: List[Union[float, int]]) -> None:
"""Records multiple observed values simultaneously.

Args:
metric: A list containing the `Metric` s to be recorded
values: A list containing the values to record
"""


class LabelKey:
"""The label keys associated with the metric.

:type key: str
:param key: the key for the label

:type description: str
:param description: description of the label
"""
def __init__(self,
key: str,
description: str) -> None:
self.key = key
self.description = description


class LabelValue:
"""The label values associated with a TimeSeries.

:type value: str
:param value: the value for the label
"""
def __init__(self,
value: str) -> None:
self.value = value
Copy link

Choose a reason for hiding this comment

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

I wonder if there's any good reasons to have mandatory containers for label values? Currently it seems to be engineered like that just for the sake of completeness.

As far as I understand majority of industry solutions, there's no support for anything but strings for labels, and it's unlikely to change in the years to come.

Copy link
Contributor Author

@lzchen lzchen Sep 3, 2019

Choose a reason for hiding this comment

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

This is good point and a common ask. I think it will make sense to simply use strings for the label values.

Copy link

Choose a reason for hiding this comment

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

I think each language should decide how to handle this question. If there is some kind of built-in support for key-value in the language, they should prefer that. In the Go repository, we have a common core.KeyValue type that can be used as both an event label and a metric label. In logging (events), it's more common to have non-string values.

I'm not sure the API needs to specify anything about how a vendor should behave when, say, a numeric value is passed as the value for a label (and the compiler didn't prevent it). I would say the default behavior should coerce values to a string silently, not something to worry about.

Loading