From e333bea862b454a96a2481cc56735ed5efef6601 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Mon, 7 Aug 2023 11:39:10 +0200 Subject: [PATCH] code review comments --- .../metrics/internal/descriptor/Advice.java | 4 ++ .../metrics/internal/view/ViewRegistry.java | 44 ++++++++----------- .../internal/view/ViewRegistryTest.java | 35 ++++++++++++++- 3 files changed, 56 insertions(+), 27 deletions(-) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/Advice.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/Advice.java index f9a2f98399a..96302ecfbe4 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/Advice.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/Advice.java @@ -35,6 +35,10 @@ public static AdviceBuilder builder() { @Nullable public abstract List> getAttributes(); + public boolean hasAttributes() { + return getAttributes() != null; + } + @AutoValue.Builder public abstract static class AdviceBuilder { diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistry.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistry.java index 8b1ce8b1284..9e5bba37233 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistry.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistry.java @@ -130,32 +130,30 @@ public List findViews( RegisteredView instrumentDefaultView = requireNonNull(instrumentDefaultRegisteredView.get(descriptor.getType())); - // if the user defined an advice, use it - if (shouldApplyAdvice(descriptor.getAdvice())) { - instrumentDefaultView = - applyAdviceToDefaultView(instrumentDefaultView, descriptor.getAdvice()); - } - AggregatorFactory viewAggregatorFactory = (AggregatorFactory) instrumentDefaultView.getView().getAggregation(); - // If the aggregation from default aggregation selector is compatible with the instrument, use - // it - if (viewAggregatorFactory.isCompatibleWithInstrument(descriptor)) { - return Collections.singletonList(instrumentDefaultView); + if (!viewAggregatorFactory.isCompatibleWithInstrument(descriptor)) { + // The aggregation from default aggregation selector was incompatible with instrument, use + // default aggregation instead + logger.log( + Level.WARNING, + "Instrument default aggregation " + + AggregationUtil.aggregationName(instrumentDefaultView.getView().getAggregation()) + + " is incompatible with instrument " + + descriptor.getName() + + " of type " + + descriptor.getType()); + instrumentDefaultView = DEFAULT_REGISTERED_VIEW; + } + + // if the user defined an attributes advice, use it + if (descriptor.getAdvice().hasAttributes()) { + instrumentDefaultView = + applyAdviceToDefaultView(instrumentDefaultView, descriptor.getAdvice()); } - // The aggregation from default aggregation selector was incompatible with instrument, use - // default aggregation instead - logger.log( - Level.WARNING, - "Instrument default aggregation " - + AggregationUtil.aggregationName(instrumentDefaultView.getView().getAggregation()) - + " is incompatible with instrument " - + descriptor.getName() - + " of type " - + descriptor.getType()); - return Collections.singletonList(DEFAULT_REGISTERED_VIEW); + return Collections.singletonList(instrumentDefaultView); } // Matches an instrument selector against an instrument + meter. @@ -255,10 +253,6 @@ private static Pattern toRegexPattern(String globPattern) { return Pattern.compile(patternBuilder.toString()); } - private static boolean shouldApplyAdvice(Advice descriptor) { - return descriptor.getAttributes() != null; - } - private static RegisteredView applyAdviceToDefaultView( RegisteredView instrumentDefaultView, Advice advice) { return RegisteredView.create( diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistryTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistryTest.java index 79046c8c7d2..39d273be674 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistryTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/ViewRegistryTest.java @@ -434,14 +434,21 @@ void defaults() { } @Test - void adviceAttributesProcessor() { + void findViews_ApplyAdvice() { + // use incompatible aggregation for histogram + DefaultAggregationSelector aggregationSelector = + instrumentType -> + instrumentType == InstrumentType.HISTOGRAM + ? Aggregation.lastValue() + : Aggregation.defaultAggregation(); + RegisteredView registeredView = registeredView( InstrumentSelector.builder().setName("test").build(), View.builder().setDescription("view applied").build()); ViewRegistry viewRegistry = ViewRegistry.create( - DefaultAggregationSelector.getDefault(), + aggregationSelector, CardinalityLimitSelector.defaultCardinalityLimitSelector(), Collections.singletonList(registeredView)); @@ -483,6 +490,30 @@ void adviceAttributesProcessor() { MetricStorage.DEFAULT_MAX_CARDINALITY, SourceInfo.noSourceInfo()))); + // If there is no matching view and attributes advice was defined, use it - incompatible + // aggregation case + assertThat( + viewRegistry.findViews( + InstrumentDescriptor.create( + "histogram_advice", + "", + "", + InstrumentType.HISTOGRAM, + InstrumentValueType.DOUBLE, + Advice.builder() + .setAttributes(Arrays.asList(stringKey("key1"), stringKey("key2"))) + .build()), + INSTRUMENTATION_SCOPE_INFO)) + .isEqualTo( + Collections.singletonList( + RegisteredView.create( + DEFAULT_REGISTERED_VIEW.getInstrumentSelector(), + DEFAULT_REGISTERED_VIEW.getView(), + new AdviceAttributesProcessor( + Arrays.asList(stringKey("key1"), stringKey("key2"))), + DEFAULT_REGISTERED_VIEW.getCardinalityLimit(), + DEFAULT_REGISTERED_VIEW.getViewSourceInfo()))); + // if advice is not defined, use the default view assertThat( viewRegistry.findViews(