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

Throw an error when multiple instruments are registered by the same name #1438

Merged
merged 22 commits into from
Dec 7, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
e564154
Throw an error when multiple instruments are registered by same name
srikanthccv Nov 30, 2020
2ceca73
Add tests for same name instrumentation exception
srikanthccv Nov 30, 2020
6a4d6d6
Remove observer name from set when unregistered
srikanthccv Dec 1, 2020
9e7af7c
Fix opentelemetry-instrumentation tests
srikanthccv Dec 1, 2020
a8b76aa
Fix opencensus exporter tests
srikanthccv Dec 1, 2020
ecb773e
Add test for unregistering and re-registering
srikanthccv Dec 1, 2020
a584e55
Update opentelemetry-sdk/src/opentelemetry/sdk/metrics/__init__.py
srikanthccv Dec 1, 2020
16824c6
Update method name
srikanthccv Dec 1, 2020
588970d
appropiate naming: instrumentation -> instrument
srikanthccv Dec 1, 2020
00d02c3
Review changes
srikanthccv Dec 2, 2020
018cea1
Update .gitignore
srikanthccv Dec 2, 2020
978a2a4
Small review changes
srikanthccv Dec 3, 2020
1a5e2c4
Remove unused import Union
srikanthccv Dec 3, 2020
89ce5a5
Use one container for both metrics and observers
srikanthccv Dec 3, 2020
9d291fc
Update contrib repo SHA
srikanthccv Dec 4, 2020
3b9fdc6
Merge branch 'master' into ot-1201
srikanthccv Dec 4, 2020
d534d60
Resolve conflicts
srikanthccv Dec 4, 2020
052dfb9
Update metrics/__init__.py
srikanthccv Dec 4, 2020
72ca4be
Merge branch 'ot-1201' of https://github.com/lonewolf3739/opentelemet…
srikanthccv Dec 4, 2020
f69e12f
Merge branch 'master' into ot-1201
srikanthccv Dec 5, 2020
9131317
Update contrib repo SHA
srikanthccv Dec 5, 2020
8e5456d
Merge branch 'ot-1201' of https://github.com/lonewolf3739/opentelemet…
srikanthccv Dec 5, 2020
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
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,13 @@ def test_get_collector_metric_type(self):
def test_get_collector_point(self):
aggregator = aggregate.SumAggregator()
int_counter = self._meter.create_counter(
"testName", "testDescription", "unit", int,
"testNameIntCounter", "testDescription", "unit", int,
)
float_counter = self._meter.create_counter(
"testName", "testDescription", "unit", float,
"testNameFloatCounter", "testDescription", "unit", float,
)
valuerecorder = self._meter.create_valuerecorder(
"testName", "testDescription", "unit", float,
"testNameValueRecorder", "testDescription", "unit", float,
)
result = metrics_exporter.get_collector_point(
ExportRecord(
Expand Down Expand Up @@ -168,7 +168,7 @@ def test_export(self):

def test_translate_to_collector(self):
test_metric = self._meter.create_counter(
"testname", "testdesc", "unit", int, self._labels.keys()
"testcollector", "testdesc", "unit", int, self._labels.keys()
)
aggregator = aggregate.SumAggregator()
aggregator.update(123)
Expand All @@ -185,7 +185,9 @@ def test_translate_to_collector(self):
)
self.assertEqual(len(output_metrics), 1)
self.assertIsInstance(output_metrics[0], metrics_pb2.Metric)
self.assertEqual(output_metrics[0].metric_descriptor.name, "testname")
self.assertEqual(
output_metrics[0].metric_descriptor.name, "testcollector"
)
self.assertEqual(
output_metrics[0].metric_descriptor.description, "testdesc"
)
Expand Down
6 changes: 5 additions & 1 deletion opentelemetry-instrumentation/tests/test_metric.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def test_ctor(self):
"measures the duration of the outbound HTTP request",
)

def test_ctor_types(self):
def test_ctor_type_client(self):
meter = metrics_api.get_meter(__name__)
recorder = HTTPMetricRecorder(meter, HTTPMetricType.CLIENT)
self.assertEqual(recorder._http_type, HTTPMetricType.CLIENT)
Expand All @@ -81,13 +81,17 @@ def test_ctor_types(self):
)
self.assertIsNone(recorder._server_duration)

def test_ctor_type_server(self):
meter = metrics_api.get_meter(__name__)
recorder = HTTPMetricRecorder(meter, HTTPMetricType.SERVER)
self.assertEqual(recorder._http_type, HTTPMetricType.SERVER)
self.assertTrue(
isinstance(recorder._server_duration, metrics.ValueRecorder)
)
self.assertIsNone(recorder._client_duration)

def test_ctor_type_both(self):
meter = metrics_api.get_meter(__name__)
recorder = HTTPMetricRecorder(meter, HTTPMetricType.BOTH)
self.assertEqual(recorder._http_type, HTTPMetricType.BOTH)
self.assertTrue(
Expand Down
15 changes: 15 additions & 0 deletions opentelemetry-sdk/src/opentelemetry/sdk/metrics/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,14 @@ def __init__(
self.metrics_lock = threading.Lock()
self.observers_lock = threading.Lock()
self.view_manager = ViewManager()
self.instrumentation_names = set()
srikanthccv marked this conversation as resolved.
Show resolved Hide resolved

def _check_instrumentation_name(self, name: str):
srikanthccv marked this conversation as resolved.
Show resolved Hide resolved
if name in self.instrumentation_names:
srikanthccv marked this conversation as resolved.
Show resolved Hide resolved
raise ValueError(
"Multiple instruments can't registered by the same name"
srikanthccv marked this conversation as resolved.
Show resolved Hide resolved
)
self.instrumentation_names.add(name)

def collect(self) -> None:
"""Collects all the metrics created with this `Meter` for export.
Expand Down Expand Up @@ -429,6 +437,7 @@ def create_counter(
enabled: bool = True,
) -> metrics_api.Counter:
"""See `opentelemetry.metrics.Meter.create_counter`."""
self._check_instrumentation_name(name)
counter = Counter(
name, description, unit, value_type, self, enabled=enabled
)
Expand All @@ -445,6 +454,7 @@ def create_updowncounter(
enabled: bool = True,
) -> metrics_api.UpDownCounter:
"""See `opentelemetry.metrics.Meter.create_updowncounter`."""
self._check_instrumentation_name(name)
counter = UpDownCounter(
name, description, unit, value_type, self, enabled=enabled
)
Expand All @@ -461,6 +471,7 @@ def create_valuerecorder(
enabled: bool = True,
) -> metrics_api.ValueRecorder:
"""See `opentelemetry.metrics.Meter.create_valuerecorder`."""
self._check_instrumentation_name(name)
recorder = ValueRecorder(
name, description, unit, value_type, self, enabled=enabled
)
Expand All @@ -478,6 +489,7 @@ def register_sumobserver(
label_keys: Sequence[str] = (),
enabled: bool = True,
) -> metrics_api.SumObserver:
self._check_instrumentation_name(name)
ob = SumObserver(
callback,
name,
Expand All @@ -502,6 +514,7 @@ def register_updownsumobserver(
label_keys: Sequence[str] = (),
enabled: bool = True,
) -> metrics_api.UpDownSumObserver:
self._check_instrumentation_name(name)
ob = UpDownSumObserver(
callback,
name,
Expand All @@ -526,6 +539,7 @@ def register_valueobserver(
label_keys: Sequence[str] = (),
enabled: bool = True,
) -> metrics_api.ValueObserver:
self._check_instrumentation_name(name)
ob = ValueObserver(
callback,
name,
Expand All @@ -542,6 +556,7 @@ def register_valueobserver(

def unregister_observer(self, observer: metrics_api.Observer) -> None:
with self.observers_lock:
self.instrumentation_names.remove(observer.name)
self.observers.remove(observer)

def register_view(self, view):
Expand Down
32 changes: 32 additions & 0 deletions opentelemetry-sdk/tests/metrics/test_metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,23 @@ def test_create_counter(self):
self.assertIs(meter_provider.resource, resource)
self.assertEqual(counter.meter, meter)

def test_instrumentation_same_name_error(self):
srikanthccv marked this conversation as resolved.
Show resolved Hide resolved
resource = Mock(spec=resources.Resource)
meter_provider = metrics.MeterProvider(resource=resource)
meter = meter_provider.get_meter(__name__)
counter = meter.create_counter("name", "desc", "unit", int,)
self.assertIsInstance(counter, metrics.Counter)
self.assertEqual(counter.value_type, int)
self.assertEqual(counter.name, "name")
self.assertIs(meter_provider.resource, resource)
self.assertEqual(counter.meter, meter)
with self.assertRaises(ValueError) as ctx:
_ = meter.create_counter("name", "desc", "unit", int,)
self.assertTrue(
"Multiple instruments can't registered by the same name"
in str(ctx.exception)
)

def test_create_updowncounter(self):
meter = metrics.MeterProvider().get_meter(__name__)
updowncounter = meter.create_updowncounter(
Expand Down Expand Up @@ -255,6 +272,21 @@ def test_unregister_observer(self):
meter.unregister_observer(observer)
self.assertEqual(len(meter.observers), 0)

def test_unregister_and_reregister_observer(self):
meter = metrics.MeterProvider().get_meter(__name__)

callback = Mock()

observer = meter.register_valueobserver(
callback, "name", "desc", "unit", int, metrics.ValueObserver
)

meter.unregister_observer(observer)
self.assertEqual(len(meter.observers), 0)
observer = meter.register_valueobserver(
callback, "name", "desc", "unit", int, metrics.ValueObserver
)


class TestMetric(unittest.TestCase):
def test_bind(self):
Expand Down