Skip to content

Commit afd4e2c

Browse files
czhang771czhang771
committed
address basic PR comments
1 parent 351730c commit afd4e2c

File tree

7 files changed

+72
-73
lines changed

7 files changed

+72
-73
lines changed

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

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ def collect(
117117
def _collect_exemplars(self) -> Sequence[Exemplar]:
118118
return self._reservoir.collect(
119119
self._attributes
120-
) # FIXME provide filtered data point attributes
120+
)
121121

122122

123123
class _DropAggregation(_Aggregation):
@@ -165,7 +165,7 @@ def aggregate(
165165

166166
self._value = self._value + measurement.value
167167

168-
super().aggregate(measurement, should_sample_exemplar)
168+
super().aggregate(measurement, should_sample_exemplar)
169169

170170
def collect(
171171
self,
@@ -295,7 +295,6 @@ def collect(
295295
with self._lock:
296296
value = self._value
297297
self._value = None
298-
exemplars = self._collect_exemplars()
299298

300299
if (
301300
self._instrument_aggregation_temporality
@@ -320,7 +319,7 @@ def collect(
320319

321320
return NumberDataPoint(
322321
attributes=self._attributes,
323-
exemplars=exemplars,
322+
exemplars=self._collect_exemplars(),
324323
start_time_unix_nano=previous_collection_start_nano,
325324
time_unix_nano=collection_start_nano,
326325
value=value,
@@ -333,7 +332,7 @@ def collect(
333332

334333
return NumberDataPoint(
335334
attributes=self._attributes,
336-
exemplars=exemplars,
335+
exemplars=self._collect_exemplars(),
337336
start_time_unix_nano=self._start_time_unix_nano,
338337
time_unix_nano=collection_start_nano,
339338
value=self._previous_value,
@@ -362,15 +361,15 @@ def collect(
362361

363362
return NumberDataPoint(
364363
attributes=self._attributes,
365-
exemplars=exemplars,
364+
exemplars=self._collect_exemplars(),
366365
start_time_unix_nano=previous_collection_start_nano,
367366
time_unix_nano=collection_start_nano,
368367
value=result_value,
369368
)
370369

371370
return NumberDataPoint(
372371
attributes=self._attributes,
373-
exemplars=exemplars,
372+
exemplars=self._collect_exemplars(),
374373
start_time_unix_nano=self._start_time_unix_nano,
375374
time_unix_nano=collection_start_nano,
376375
value=value,

‎opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/exemplar/exemplar_reservoir.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ def collect(self, point_attributes: Attributes) -> Exemplar | None:
9898
if not self.__offered:
9999
return None
100100

101-
current_attributes = (
101+
filtered_attributes = (
102102
{
103103
k: v
104104
for k, v in self.__attributes.items()
@@ -109,7 +109,7 @@ def collect(self, point_attributes: Attributes) -> Exemplar | None:
109109
)
110110

111111
exemplar = Exemplar(
112-
current_attributes,
112+
filtered_attributes,
113113
self.__value,
114114
self.__time_unix_nano,
115115
self.__span_id,
@@ -206,11 +206,11 @@ def _find_bucket_index(
206206
attributes: Attributes,
207207
ctx: Context,
208208
) -> int:
209-
self._measurements_seen += 1
210209
if self._measurements_seen < self._size:
211210
return self._measurements_seen
212211

213212
index = randrange(0, self._measurements_seen)
213+
self._measurements_seen += 1
214214
return index if index < self._size else -1
215215

216216

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ class Measurement:
2828
Attributes:
2929
value: Measured value
3030
time_unix_nano: The time the API call was made to record the Measurement
31-
instrument: Measurement instrument
31+
instrument: The instrument that produced this `Measurement`.
3232
context: The active Context of the Measurement at API call time.
3333
attributes: Measurement attributes
3434
"""

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@
3434
_logger = getLogger(__name__)
3535

3636

37-
def default_reservoir_factory(
37+
def _default_reservoir_factory(
3838
aggregationType: Type[_Aggregation],
3939
) -> ExemplarReservoirFactory:
4040
"""Default reservoir factory per aggregation."""
@@ -159,7 +159,7 @@ def __init__(
159159
self._attribute_keys = attribute_keys
160160
self._aggregation = aggregation or self._default_aggregation
161161
self._exemplar_reservoir_factory = (
162-
exemplar_reservoir_factory or default_reservoir_factory
162+
exemplar_reservoir_factory or _default_reservoir_factory
163163
)
164164

165165
# pylint: disable=too-many-return-statements

‎opentelemetry-sdk/tests/metrics/exponential_histogram/test_exponential_bucket_histogram_aggregation.py‎

Lines changed: 25 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@
4747
from opentelemetry.sdk.metrics._internal.point import (
4848
ExponentialHistogramDataPoint,
4949
)
50-
from opentelemetry.sdk.metrics._internal.view import default_reservoir_factory
50+
from opentelemetry.sdk.metrics._internal.view import _default_reservoir_factory
5151
from opentelemetry.sdk.metrics.view import (
5252
ExponentialBucketHistogramAggregation,
5353
)
@@ -168,7 +168,7 @@ def test_alternating_growth_0(self):
168168
exponential_histogram_aggregation = (
169169
_ExponentialBucketHistogramAggregation(
170170
Mock(),
171-
default_reservoir_factory(
171+
_default_reservoir_factory(
172172
_ExponentialBucketHistogramAggregation
173173
),
174174
AggregationTemporality.DELTA,
@@ -209,7 +209,7 @@ def test_alternating_growth_1(self):
209209
exponential_histogram_aggregation = (
210210
_ExponentialBucketHistogramAggregation(
211211
Mock(),
212-
default_reservoir_factory(
212+
_default_reservoir_factory(
213213
_ExponentialBucketHistogramAggregation
214214
),
215215
AggregationTemporality.DELTA,
@@ -292,7 +292,7 @@ def test_permutations(self):
292292
exponential_histogram_aggregation = (
293293
_ExponentialBucketHistogramAggregation(
294294
Mock(),
295-
default_reservoir_factory(
295+
_default_reservoir_factory(
296296
_ExponentialBucketHistogramAggregation
297297
),
298298
AggregationTemporality.DELTA,
@@ -342,7 +342,7 @@ def ascending_sequence_test(
342342
exponential_histogram_aggregation = (
343343
_ExponentialBucketHistogramAggregation(
344344
Mock(),
345-
default_reservoir_factory(
345+
_default_reservoir_factory(
346346
_ExponentialBucketHistogramAggregation
347347
),
348348
AggregationTemporality.DELTA,
@@ -455,7 +455,7 @@ def mock_increment(self, bucket_index: int) -> None:
455455
exponential_histogram_aggregation = (
456456
_ExponentialBucketHistogramAggregation(
457457
Mock(),
458-
default_reservoir_factory(
458+
_default_reservoir_factory(
459459
_ExponentialBucketHistogramAggregation
460460
),
461461
AggregationTemporality.DELTA,
@@ -521,7 +521,7 @@ def test_move_into(self):
521521
exponential_histogram_aggregation_0 = (
522522
_ExponentialBucketHistogramAggregation(
523523
Mock(),
524-
default_reservoir_factory(
524+
_default_reservoir_factory(
525525
_ExponentialBucketHistogramAggregation
526526
),
527527
AggregationTemporality.DELTA,
@@ -532,7 +532,7 @@ def test_move_into(self):
532532
exponential_histogram_aggregation_1 = (
533533
_ExponentialBucketHistogramAggregation(
534534
Mock(),
535-
default_reservoir_factory(
535+
_default_reservoir_factory(
536536
_ExponentialBucketHistogramAggregation
537537
),
538538
AggregationTemporality.DELTA,
@@ -589,7 +589,7 @@ def test_very_large_numbers(self):
589589
exponential_histogram_aggregation = (
590590
_ExponentialBucketHistogramAggregation(
591591
Mock(),
592-
default_reservoir_factory(
592+
_default_reservoir_factory(
593593
_ExponentialBucketHistogramAggregation
594594
),
595595
AggregationTemporality.DELTA,
@@ -675,7 +675,7 @@ def test_full_range(self):
675675
exponential_histogram_aggregation = (
676676
_ExponentialBucketHistogramAggregation(
677677
Mock(),
678-
default_reservoir_factory(
678+
_default_reservoir_factory(
679679
_ExponentialBucketHistogramAggregation
680680
),
681681
AggregationTemporality.DELTA,
@@ -723,7 +723,7 @@ def test_aggregator_min_max(self):
723723
exponential_histogram_aggregation = (
724724
_ExponentialBucketHistogramAggregation(
725725
Mock(),
726-
default_reservoir_factory(
726+
_default_reservoir_factory(
727727
_ExponentialBucketHistogramAggregation
728728
),
729729
AggregationTemporality.DELTA,
@@ -742,7 +742,7 @@ def test_aggregator_min_max(self):
742742
exponential_histogram_aggregation = (
743743
_ExponentialBucketHistogramAggregation(
744744
Mock(),
745-
default_reservoir_factory(
745+
_default_reservoir_factory(
746746
_ExponentialBucketHistogramAggregation
747747
),
748748
AggregationTemporality.DELTA,
@@ -764,7 +764,7 @@ def test_aggregator_copy_swap(self):
764764
exponential_histogram_aggregation_0 = (
765765
_ExponentialBucketHistogramAggregation(
766766
Mock(),
767-
default_reservoir_factory(
767+
_default_reservoir_factory(
768768
_ExponentialBucketHistogramAggregation
769769
),
770770
AggregationTemporality.DELTA,
@@ -778,7 +778,7 @@ def test_aggregator_copy_swap(self):
778778
exponential_histogram_aggregation_1 = (
779779
_ExponentialBucketHistogramAggregation(
780780
Mock(),
781-
default_reservoir_factory(
781+
_default_reservoir_factory(
782782
_ExponentialBucketHistogramAggregation
783783
),
784784
AggregationTemporality.DELTA,
@@ -792,7 +792,7 @@ def test_aggregator_copy_swap(self):
792792
exponential_histogram_aggregation_2 = (
793793
_ExponentialBucketHistogramAggregation(
794794
Mock(),
795-
default_reservoir_factory(
795+
_default_reservoir_factory(
796796
_ExponentialBucketHistogramAggregation
797797
),
798798
AggregationTemporality.DELTA,
@@ -845,7 +845,7 @@ def test_zero_count_by_increment(self):
845845
exponential_histogram_aggregation_0 = (
846846
_ExponentialBucketHistogramAggregation(
847847
Mock(),
848-
default_reservoir_factory(
848+
_default_reservoir_factory(
849849
_ExponentialBucketHistogramAggregation
850850
),
851851
AggregationTemporality.DELTA,
@@ -862,7 +862,7 @@ def test_zero_count_by_increment(self):
862862
exponential_histogram_aggregation_1 = (
863863
_ExponentialBucketHistogramAggregation(
864864
Mock(),
865-
default_reservoir_factory(
865+
_default_reservoir_factory(
866866
_ExponentialBucketHistogramAggregation
867867
),
868868
AggregationTemporality.DELTA,
@@ -905,7 +905,7 @@ def test_one_count_by_increment(self):
905905
exponential_histogram_aggregation_0 = (
906906
_ExponentialBucketHistogramAggregation(
907907
Mock(),
908-
default_reservoir_factory(
908+
_default_reservoir_factory(
909909
_ExponentialBucketHistogramAggregation
910910
),
911911
AggregationTemporality.DELTA,
@@ -922,7 +922,7 @@ def test_one_count_by_increment(self):
922922
exponential_histogram_aggregation_1 = (
923923
_ExponentialBucketHistogramAggregation(
924924
Mock(),
925-
default_reservoir_factory(
925+
_default_reservoir_factory(
926926
_ExponentialBucketHistogramAggregation
927927
),
928928
AggregationTemporality.DELTA,
@@ -996,7 +996,7 @@ def test_min_max_size(self):
996996
exponential_histogram_aggregation = (
997997
_ExponentialBucketHistogramAggregation(
998998
Mock(),
999-
default_reservoir_factory(
999+
_default_reservoir_factory(
10001000
_ExponentialBucketHistogramAggregation
10011001
),
10021002
AggregationTemporality.DELTA,
@@ -1027,7 +1027,7 @@ def test_aggregate_collect(self):
10271027
exponential_histogram_aggregation = (
10281028
_ExponentialBucketHistogramAggregation(
10291029
Mock(),
1030-
default_reservoir_factory(
1030+
_default_reservoir_factory(
10311031
_ExponentialBucketHistogramAggregation
10321032
),
10331033
AggregationTemporality.DELTA,
@@ -1061,7 +1061,7 @@ def test_collect_results_cumulative(self) -> None:
10611061
exponential_histogram_aggregation = (
10621062
_ExponentialBucketHistogramAggregation(
10631063
Mock(),
1064-
default_reservoir_factory(
1064+
_default_reservoir_factory(
10651065
_ExponentialBucketHistogramAggregation
10661066
),
10671067
AggregationTemporality.DELTA,
@@ -1170,7 +1170,7 @@ def test_cumulative_aggregation_with_random_data(self) -> None:
11701170

11711171
histogram = _ExponentialBucketHistogramAggregation(
11721172
Mock(),
1173-
default_reservoir_factory(_ExponentialBucketHistogramAggregation),
1173+
_default_reservoir_factory(_ExponentialBucketHistogramAggregation),
11741174
AggregationTemporality.DELTA,
11751175
Mock(),
11761176
)
@@ -1232,7 +1232,7 @@ def test_merge_collect_cumulative(self):
12321232
exponential_histogram_aggregation = (
12331233
_ExponentialBucketHistogramAggregation(
12341234
Mock(),
1235-
default_reservoir_factory(
1235+
_default_reservoir_factory(
12361236
_ExponentialBucketHistogramAggregation
12371237
),
12381238
AggregationTemporality.DELTA,
@@ -1290,7 +1290,7 @@ def test_merge_collect_delta(self):
12901290
exponential_histogram_aggregation = (
12911291
_ExponentialBucketHistogramAggregation(
12921292
Mock(),
1293-
default_reservoir_factory(
1293+
_default_reservoir_factory(
12941294
_ExponentialBucketHistogramAggregation
12951295
),
12961296
AggregationTemporality.DELTA,

0 commit comments

Comments
 (0)