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

Move "config" out from Meter into MeterProvider #751

Merged
merged 12 commits into from
Jun 4, 2020
12 changes: 6 additions & 6 deletions docs/examples/basic_meter/basic_metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,13 @@ def usage(argv):
)


# Stateful determines whether how metrics are collected: if true, metrics
lzchen marked this conversation as resolved.
Show resolved Hide resolved
# accumulate over the process lifetime. If false, metrics are reset at the
# beginning of each collection interval.
metrics.set_meter_provider(MeterProvider(batcher_mode == "stateful"))
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this check happens on line 46. Right now this script crashes without any arguments

Suggested change
metrics.set_meter_provider(MeterProvider(batcher_mode == "stateful"))
metrics.set_meter_provider(MeterProvider(stateful))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good question. The current design is such that the responsibility for "statefulness" belongs to the batcher, and in turn, the meter. This means that all metrics that created and recorded with this meter will have the same statefulness. This "batcher" concept was created previously when the metrics api was not as well defined (and we still don't really have a well defined SDK spec), in which we needed a way to determine whether we are calculating actual values or deltas. I think some of the optional refinements for metric instruments is trying to address these cases, in which if they become actual api elements, we will have to change our design to decouple statefulness from the batcher and make them a metric level control.

# The Meter is responsible for creating and recording metrics. Each meter has a
# unique name, which we set as the module's name here. The second argument
# determines whether how metrics are collected: if true, metrics accumulate
# over the process lifetime. If false, metrics are reset at the beginning of
# each collection interval.
metrics.set_meter_provider(MeterProvider())
meter = metrics.get_meter(__name__, batcher_mode == "stateful")
# unique name, which we set as the module's name here.
meter = metrics.get_meter(__name__)

# Exporter to export metrics to the console
exporter = ConsoleMetricsExporter()
Expand Down
2 changes: 0 additions & 2 deletions docs/examples/basic_meter/observer.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@
from opentelemetry.sdk.metrics.export.batcher import UngroupedBatcher
from opentelemetry.sdk.metrics.export.controller import PushController

# Configure a stateful batcher
batcher = UngroupedBatcher(stateful=True)
metrics.set_meter_provider(MeterProvider())
meter = metrics.get_meter(__name__)
exporter = ConsoleMetricsExporter()
Expand Down
20 changes: 8 additions & 12 deletions opentelemetry-api/src/opentelemetry/metrics/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
"""
import abc
from logging import getLogger
from typing import Callable, Dict, Sequence, Tuple, Type, TypeVar
from typing import Callable, Dict, Optional, Sequence, Tuple, Type, TypeVar

from opentelemetry.util import _load_provider

Expand Down Expand Up @@ -206,7 +206,6 @@ class MeterProvider(abc.ABC):
def get_meter(
self,
instrumenting_module_name: str,
stateful: bool = True,
instrumenting_library_version: str = "",
) -> "Meter":
"""Returns a `Meter` for use by the given instrumentation library.
Expand All @@ -223,12 +222,6 @@ def get_meter(
E.g., instead of ``"requests"``, use
``"opentelemetry.ext.requests"``.

stateful: True/False to indicate whether the meter will be
stateful. True indicates the meter computes checkpoints
from over the process lifetime. False indicates the meter
computes checkpoints which describe the updates of a single
collection period (deltas).

instrumenting_library_version: Optional. The version string of the
instrumenting library. Usually this should be the same as
``pkg_resources.get_distribution(instrumenting_library_name).version``.
Expand All @@ -244,7 +237,6 @@ class DefaultMeterProvider(MeterProvider):
def get_meter(
self,
instrumenting_module_name: str,
stateful: bool = True,
instrumenting_library_version: str = "",
) -> "Meter":
# pylint:disable=no-self-use,unused-argument
Expand Down Expand Up @@ -387,15 +379,19 @@ def unregister_observer(self, observer: "Observer") -> None:

def get_meter(
instrumenting_module_name: str,
stateful: bool = True,
instrumenting_library_version: str = "",
meter_provider: Optional[MeterProvider] = None,
) -> "Meter":
"""Returns a `Meter` for use by the given instrumentation library.
This function is a convenience wrapper for
opentelemetry.metrics.get_meter_provider().get_meter

If meter_provider is omitted the current configured one is used.
"""
return get_meter_provider().get_meter(
instrumenting_module_name, stateful, instrumenting_library_version
if meter_provider is None:
meter_provider = get_meter_provider()
return meter_provider.get_meter(
instrumenting_module_name, instrumenting_library_version
)


Expand Down
25 changes: 16 additions & 9 deletions opentelemetry-sdk/src/opentelemetry/sdk/metrics/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -270,22 +270,21 @@ class Meter(metrics_api.Meter):
"""See `opentelemetry.metrics.Meter`.

Args:
source: The `MeterProvider` that created this meter.
instrumentation_info: The `InstrumentationInfo` for this meter.
stateful: Indicates whether the meter is stateful.
"""

def __init__(
self,
source: "MeterProvider",
instrumentation_info: "InstrumentationInfo",
stateful: bool,
resource: Resource = Resource.create_empty(),
):
self.instrumentation_info = instrumentation_info
self.batcher = UngroupedBatcher(source.stateful)
self.resource = source.resource
self.metrics = set()
self.observers = set()
self.batcher = UngroupedBatcher(stateful)
self.observers_lock = threading.Lock()
self.resource = resource

def collect(self) -> None:
"""Collects all the metrics created with this `Meter` for export.
Expand Down Expand Up @@ -398,21 +397,29 @@ def unregister_observer(self, observer: "Observer") -> None:


class MeterProvider(metrics_api.MeterProvider):
def __init__(self, resource: Resource = Resource.create_empty()):
"""See `opentelemetry.metrics.MeterProvider`.

Args:
stateful: Indicates whether meters created are going to be stateful
resource: Resource for this MeterProvider
"""

def __init__(
self, stateful=True, resource: Resource = Resource.create_empty()
lzchen marked this conversation as resolved.
Show resolved Hide resolved
):
self.stateful = stateful
self.resource = resource

def get_meter(
self,
instrumenting_module_name: str,
stateful=True,
instrumenting_library_version: str = "",
) -> "metrics_api.Meter":
if not instrumenting_module_name: # Reject empty strings too.
raise ValueError("get_meter called with missing module name.")
return Meter(
self,
InstrumentationInfo(
instrumenting_module_name, instrumenting_library_version
),
stateful=stateful,
resource=self.resource,
)
5 changes: 5 additions & 0 deletions opentelemetry-sdk/tests/metrics/test_metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@


class TestMeterProvider(unittest.TestCase):
def test_stateful(self):
meter_provider = metrics.MeterProvider(stateful=False)
meter = meter_provider.get_meter(__name__)
self.assertIs(meter.batcher.stateful, False)

def test_resource(self):
resource = resources.Resource.create({})
meter_provider = metrics.MeterProvider(resource=resource)
Expand Down