Skip to content

Commit 0a565b1

Browse files
ocelotlocelotl
committed
Refactor view conflict handling
Fixes open-telemetry#2558
1 parent 43288ca commit 0a565b1

File tree

2 files changed

+269
-142
lines changed

2 files changed

+269
-142
lines changed

‎opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py

+178-88
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,8 @@ def __init__(
6767
) -> None:
6868
self._lock = RLock()
6969
self._sdk_config = sdk_config
70-
self._instrument_view_instrument_matches: Dict[
71-
Instrument, List[_ViewInstrumentMatch]
70+
self._instrumentation_scope_instrument_view_instrument_matches: Dict[
71+
InstrumentationScope, Dict[Instrument, List[_ViewInstrumentMatch]]
7272
] = {}
7373
self._instrument_class_temporality = instrument_class_temporality
7474
self._instrument_class_aggregation = instrument_class_aggregation
@@ -79,13 +79,33 @@ def _get_or_init_view_instrument_match(
7979
# Optimistically get the relevant views for the given instrument. Once set for a given
8080
# instrument, the mapping will never change
8181

82-
if instrument in self._instrument_view_instrument_matches:
83-
return self._instrument_view_instrument_matches[instrument]
82+
if instrument.instrumentation_scope in (
83+
self._instrumentation_scope_instrument_view_instrument_matches
84+
) and instrument in (
85+
self._instrumentation_scope_instrument_view_instrument_matches[
86+
instrument.instrumentation_scope
87+
]
88+
):
89+
return (
90+
self._instrumentation_scope_instrument_view_instrument_matches[
91+
instrument.instrumentation_scope
92+
][instrument]
93+
)
8494

8595
with self._lock:
8696
# double check if it was set before we held the lock
87-
if instrument in self._instrument_view_instrument_matches:
88-
return self._instrument_view_instrument_matches[instrument]
97+
if instrument.instrumentation_scope in (
98+
self._instrumentation_scope_instrument_view_instrument_matches
99+
) and instrument in (
100+
self._instrumentation_scope_instrument_view_instrument_matches[
101+
instrument.instrumentation_scope
102+
]
103+
):
104+
return self._instrumentation_scope_instrument_view_instrument_matches[
105+
instrument.instrumentation_scope
106+
][
107+
instrument
108+
]
89109

90110
# not present, hold the lock and add a new mapping
91111
view_instrument_matches = []
@@ -105,9 +125,6 @@ def _get_or_init_view_instrument_match(
105125
),
106126
)
107127
)
108-
self._instrument_view_instrument_matches[
109-
instrument
110-
] = view_instrument_matches
111128

112129
return view_instrument_matches
113130

@@ -139,83 +156,93 @@ def collect(self) -> MetricsData:
139156
) = {}
140157

141158
for (
142-
instrument,
143-
view_instrument_matches,
144-
) in self._instrument_view_instrument_matches.items():
145-
aggregation_temporality = self._instrument_class_temporality[
146-
instrument.__class__
147-
]
159+
instrument_view_instrument_matches
160+
) in (
161+
self._instrumentation_scope_instrument_view_instrument_matches.values()
162+
):
163+
for (
164+
instrument,
165+
view_instrument_matches,
166+
) in instrument_view_instrument_matches.items():
167+
aggregation_temporality = (
168+
self._instrument_class_temporality[
169+
instrument.__class__
170+
]
171+
)
148172

149-
metrics: List[Metric] = []
173+
metrics: List[Metric] = []
150174

151-
for view_instrument_match in view_instrument_matches:
175+
for view_instrument_match in view_instrument_matches:
152176

153-
if isinstance(
154-
# pylint: disable=protected-access
155-
view_instrument_match._aggregation,
156-
_SumAggregation,
157-
):
158-
data = Sum(
159-
aggregation_temporality=aggregation_temporality,
160-
data_points=view_instrument_match.collect(
161-
aggregation_temporality, collection_start_nanos
162-
),
163-
is_monotonic=isinstance(
164-
instrument, (Counter, ObservableCounter)
165-
),
166-
)
167-
elif isinstance(
168-
# pylint: disable=protected-access
169-
view_instrument_match._aggregation,
170-
_LastValueAggregation,
171-
):
172-
data = Gauge(
173-
data_points=view_instrument_match.collect(
174-
aggregation_temporality, collection_start_nanos
177+
if isinstance(
178+
# pylint: disable=protected-access
179+
view_instrument_match._aggregation,
180+
_SumAggregation,
181+
):
182+
data = Sum(
183+
aggregation_temporality=aggregation_temporality,
184+
data_points=view_instrument_match.collect(
185+
aggregation_temporality,
186+
collection_start_nanos,
187+
),
188+
is_monotonic=isinstance(
189+
instrument, (Counter, ObservableCounter)
190+
),
175191
)
176-
)
177-
elif isinstance(
178-
# pylint: disable=protected-access
179-
view_instrument_match._aggregation,
180-
_ExplicitBucketHistogramAggregation,
181-
):
182-
data = Histogram(
183-
data_points=view_instrument_match.collect(
184-
aggregation_temporality, collection_start_nanos
185-
),
186-
aggregation_temporality=aggregation_temporality,
187-
)
188-
elif isinstance(
189-
# pylint: disable=protected-access
190-
view_instrument_match._aggregation,
191-
_DropAggregation,
192-
):
193-
continue
194-
195-
metrics.append(
196-
Metric(
192+
elif isinstance(
197193
# pylint: disable=protected-access
198-
name=view_instrument_match._name,
199-
description=view_instrument_match._description,
200-
unit=view_instrument_match._instrument.unit,
201-
data=data,
194+
view_instrument_match._aggregation,
195+
_LastValueAggregation,
196+
):
197+
data = Gauge(
198+
data_points=view_instrument_match.collect(
199+
aggregation_temporality,
200+
collection_start_nanos,
201+
)
202+
)
203+
elif isinstance(
204+
# pylint: disable=protected-access
205+
view_instrument_match._aggregation,
206+
_ExplicitBucketHistogramAggregation,
207+
):
208+
data = Histogram(
209+
data_points=view_instrument_match.collect(
210+
aggregation_temporality,
211+
collection_start_nanos,
212+
),
213+
aggregation_temporality=aggregation_temporality,
214+
)
215+
elif isinstance(
216+
# pylint: disable=protected-access
217+
view_instrument_match._aggregation,
218+
_DropAggregation,
219+
):
220+
continue
221+
222+
metrics.append(
223+
Metric(
224+
# pylint: disable=protected-access
225+
name=view_instrument_match._name,
226+
description=view_instrument_match._description,
227+
unit=view_instrument_match._instrument.unit,
228+
data=data,
229+
)
202230
)
203-
)
204231

205-
if instrument.instrumentation_scope not in (
206-
instrumentation_scope_scope_metrics
207-
):
208-
instrumentation_scope_scope_metrics[
209-
instrument.instrumentation_scope
210-
] = ScopeMetrics(
211-
scope=instrument.instrumentation_scope,
212-
metrics=metrics,
213-
schema_url=instrument.instrumentation_scope.schema_url,
214-
)
215-
else:
216-
instrumentation_scope_scope_metrics[
217-
instrument.instrumentation_scope
218-
].metrics.extend(metrics)
232+
if instrument.instrumentation_scope not in (
233+
instrumentation_scope_scope_metrics
234+
):
235+
instrumentation_scope_scope_metrics[
236+
instrument.instrumentation_scope
237+
] = ScopeMetrics(
238+
scope=instrument.instrumentation_scope,
239+
metrics=metrics,
240+
schema_url=instrument.instrumentation_scope.schema_url,
241+
)
242+
else:
243+
instrumentation_scope_scope_metrics[
244+
instrument.instrumentation_scope
245+
].metrics.extend(metrics)
219246

220247
return MetricsData(
221248
resource_metrics=[
@@ -229,11 +256,30 @@ def collect(self) -> MetricsData:
229256
]
230257
)
231258

259+
# pylint: disable=too-many-branches
232260
def _handle_view_instrument_match(
233261
self,
234262
instrument: Instrument,
235263
view_instrument_matches: List["_ViewInstrumentMatch"],
236264
) -> None:
265+
266+
if instrument.instrumentation_scope not in (
267+
self._instrumentation_scope_instrument_view_instrument_matches
268+
):
269+
self._instrumentation_scope_instrument_view_instrument_matches[
270+
instrument.instrumentation_scope
271+
] = {}
272+
273+
if instrument not in (
274+
self._instrumentation_scope_instrument_view_instrument_matches[
275+
instrument.instrumentation_scope
276+
]
277+
):
278+
self._instrumentation_scope_instrument_view_instrument_matches[
279+
instrument.instrumentation_scope
280+
][instrument] = view_instrument_matches
281+
282+
# pylint: disable=too-many-nested-blocks
237283
for view in self._sdk_config.views:
238284
# pylint: disable=protected-access
239285
if not view._match(instrument):
@@ -252,20 +298,64 @@ def _handle_view_instrument_match(
252298

253299
for (
254300
existing_view_instrument_matches
255-
) in self._instrument_view_instrument_matches.values():
301+
) in self._instrumentation_scope_instrument_view_instrument_matches[
302+
instrument.instrumentation_scope
303+
].values():
256304
for (
257305
existing_view_instrument_match
258306
) in existing_view_instrument_matches:
259-
if existing_view_instrument_match.conflicts(
260-
new_view_instrument_match
307+
308+
conflict_messages = []
309+
310+
if (
311+
existing_view_instrument_match._name
312+
== new_view_instrument_match._name
261313
):
262314

263-
_logger.warning(
264-
"Views %s and %s will cause conflicting "
265-
"metrics identities",
266-
existing_view_instrument_match._view,
267-
new_view_instrument_match._view,
268-
)
315+
if (
316+
existing_view_instrument_match._description
317+
!= new_view_instrument_match._description
318+
):
319+
conflict_messages.append("description")
320+
321+
# The aggregation class is being used here instead of
322+
# data point type since they are functionally
323+
# equivalent.
324+
if (
325+
existing_view_instrument_match._aggregation.__class__
326+
!= new_view_instrument_match._aggregation.__class__
327+
):
328+
conflict_messages.append("aggregation")
329+
330+
elif isinstance(
331+
existing_view_instrument_match._aggregation,
332+
_SumAggregation,
333+
):
334+
if (
335+
existing_view_instrument_match._aggregation._instrument_is_monotonic
336+
!= new_view_instrument_match._aggregation._instrument_is_monotonic
337+
):
338+
conflict_messages.append("monotonicity")
339+
340+
if (
341+
existing_view_instrument_match._aggregation._instrument_temporality
342+
!= new_view_instrument_match._aggregation._instrument_temporality
343+
):
344+
conflict_messages.append(
345+
"aggregation temporality"
346+
)
347+
348+
if conflict_messages:
349+
350+
_logger.warning(
351+
"Views named %s will cause conflicting "
352+
"metrics identities for instrument %s because "
353+
"of different %s. Rename a view named %s.",
354+
existing_view_instrument_match._view._name,
355+
instrument.name,
356+
",".join(conflict_messages),
357+
new_view_instrument_match._view._name,
358+
)
269359

270360
view_instrument_matches.append(new_view_instrument_match)
271361

0 commit comments

Comments
 (0)