Skip to content

Commit 2e385f8

Browse files
jack-bergjack-berg
authored
Enforce IncludedExcludeModel .included and .excluded are not empty (#8266)
1 parent b665652 commit 2e385f8

File tree

9 files changed

+235
-30
lines changed

9 files changed

+235
-30
lines changed

‎exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/internal/PrometheusComponentProvider.java‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
package io.opentelemetry.exporter.prometheus.internal;
77

8+
import io.opentelemetry.api.incubator.config.DeclarativeConfigException;
89
import io.opentelemetry.api.incubator.config.DeclarativeConfigProperties;
910
import io.opentelemetry.exporter.prometheus.PrometheusHttpServer;
1011
import io.opentelemetry.exporter.prometheus.PrometheusHttpServerBuilder;
@@ -59,10 +60,14 @@ public MetricReader create(DeclarativeConfigProperties config) {
5960
if (withResourceConstantLabels != null) {
6061
List<String> included = withResourceConstantLabels.getScalarList("included", String.class);
6162
List<String> excluded = withResourceConstantLabels.getScalarList("excluded", String.class);
62-
if (included != null || excluded != null) {
63-
prometheusBuilder.setAllowedResourceAttributesFilter(
64-
IncludeExcludePredicate.createPatternMatching(included, excluded));
63+
if (included != null && included.isEmpty()) {
64+
throw new DeclarativeConfigException("included must not be empty");
6565
}
66+
if (excluded != null && excluded.isEmpty()) {
67+
throw new DeclarativeConfigException("excluded must not be empty");
68+
}
69+
prometheusBuilder.setAllowedResourceAttributesFilter(
70+
IncludeExcludePredicate.createPatternMatching(included, excluded));
6671
}
6772

6873
return prometheusBuilder.build();

‎sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/ComposableRuleBasedSamplerFactory.java‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import static java.util.stream.Collectors.toSet;
1010

1111
import io.opentelemetry.api.common.Attributes;
12+
import io.opentelemetry.api.incubator.config.DeclarativeConfigException;
1213
import io.opentelemetry.api.trace.Span;
1314
import io.opentelemetry.api.trace.SpanContext;
1415
import io.opentelemetry.api.trace.SpanKind;
@@ -86,9 +87,13 @@ private static AttributeMatcher attributeValuesMatcher(
8687
if (attributeValuesModel == null) {
8788
return null;
8889
}
90+
List<String> values = attributeValuesModel.getValues();
91+
if (values == null || values.isEmpty()) {
92+
throw new DeclarativeConfigException(".values is required and must be non-empty");
93+
}
8994
return new AttributeMatcher(
9095
requireNonNull(attributeValuesModel.getKey(), "attribute_values key"),
91-
IncludeExcludePredicate.createExactMatching(attributeValuesModel.getValues(), null));
96+
IncludeExcludePredicate.createExactMatching(values, null));
9297
}
9398

9499
@Nullable
@@ -98,10 +103,17 @@ private static AttributeMatcher attributePatternsMatcher(
98103
if (attributePatternsModel == null) {
99104
return null;
100105
}
106+
List<String> included = attributePatternsModel.getIncluded();
107+
if (included != null && included.isEmpty()) {
108+
throw new DeclarativeConfigException("included must not be empty");
109+
}
110+
List<String> excluded = attributePatternsModel.getExcluded();
111+
if (excluded != null && excluded.isEmpty()) {
112+
throw new DeclarativeConfigException("excluded must not be empty");
113+
}
101114
return new AttributeMatcher(
102115
requireNonNull(attributePatternsModel.getKey(), "attribute_patterns key"),
103-
IncludeExcludePredicate.createPatternMatching(
104-
attributePatternsModel.getIncluded(), attributePatternsModel.getExcluded()));
116+
IncludeExcludePredicate.createPatternMatching(included, excluded));
105117
}
106118

107119
// Visible for testing

‎sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/IncludeExcludeFactory.java‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.sdk.extension.incubator.fileconfig;
7+
8+
import io.opentelemetry.api.incubator.config.DeclarativeConfigException;
9+
import io.opentelemetry.sdk.common.internal.IncludeExcludePredicate;
10+
import io.opentelemetry.sdk.extension.incubator.fileconfig.internal.model.IncludeExcludeModel;
11+
import java.util.List;
12+
import java.util.function.Predicate;
13+
14+
final class IncludeExcludeFactory implements Factory<IncludeExcludeModel, Predicate<String>> {
15+
16+
private static final IncludeExcludeFactory INSTANCE = new IncludeExcludeFactory();
17+
18+
private IncludeExcludeFactory() {}
19+
20+
static IncludeExcludeFactory getInstance() {
21+
return INSTANCE;
22+
}
23+
24+
@Override
25+
public Predicate<String> create(IncludeExcludeModel model, DeclarativeConfigContext context) {
26+
List<String> included = model.getIncluded();
27+
if (included != null && included.isEmpty()) {
28+
throw new DeclarativeConfigException("included must not be empty");
29+
}
30+
List<String> excluded = model.getExcluded();
31+
if (excluded != null && excluded.isEmpty()) {
32+
throw new DeclarativeConfigException("excluded must not be empty");
33+
}
34+
35+
return IncludeExcludePredicate.createPatternMatching(included, excluded);
36+
}
37+
}

‎sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/ResourceFactory.java‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010

1111
import io.opentelemetry.api.common.Attributes;
1212
import io.opentelemetry.sdk.autoconfigure.spi.internal.DefaultConfigProperties;
13-
import io.opentelemetry.sdk.common.internal.IncludeExcludePredicate;
1413
import io.opentelemetry.sdk.extension.incubator.fileconfig.internal.model.AttributeNameValueModel;
1514
import io.opentelemetry.sdk.extension.incubator.fileconfig.internal.model.ExperimentalResourceDetectionModel;
1615
import io.opentelemetry.sdk.extension.incubator.fileconfig.internal.model.ExperimentalResourceDetectorModel;
@@ -21,7 +20,6 @@
2120
import java.util.Collections;
2221
import java.util.List;
2322
import java.util.function.Predicate;
24-
import javax.annotation.Nullable;
2523

2624
final class ResourceFactory implements Factory<ResourceModel, Resource> {
2725

@@ -49,8 +47,11 @@ public Resource create(ResourceModel model, DeclarativeConfigContext context) {
4947
}
5048
}
5149

50+
IncludeExcludeModel attributesIncludeExcludeModel = detectionModel.getAttributes();
5251
Predicate<String> detectorAttributeFilter =
53-
detectorAttributeFilter(detectionModel.getAttributes());
52+
attributesIncludeExcludeModel == null
53+
? ResourceFactory::matchAll
54+
: IncludeExcludeFactory.getInstance().create(attributesIncludeExcludeModel, context);
5455
Attributes filteredDetectedAttributes =
5556
detectedResourceBuilder.build().getAttributes().toBuilder()
5657
.removeIf(attributeKey -> !detectorAttributeFilter.test(attributeKey.getKey()))
@@ -84,13 +85,4 @@ public Resource create(ResourceModel model, DeclarativeConfigContext context) {
8485
private static boolean matchAll(String attributeKey) {
8586
return true;
8687
}
87-
88-
private static Predicate<String> detectorAttributeFilter(
89-
@Nullable IncludeExcludeModel includedExcludeModel) {
90-
if (includedExcludeModel == null) {
91-
return ResourceFactory::matchAll;
92-
}
93-
return IncludeExcludePredicate.createPatternMatching(
94-
includedExcludeModel.getIncluded(), includedExcludeModel.getExcluded());
95-
}
9688
}

‎sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/ViewFactory.java‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,10 @@
55

66
package io.opentelemetry.sdk.extension.incubator.fileconfig;
77

8-
import io.opentelemetry.sdk.common.internal.IncludeExcludePredicate;
98
import io.opentelemetry.sdk.extension.incubator.fileconfig.internal.model.IncludeExcludeModel;
109
import io.opentelemetry.sdk.extension.incubator.fileconfig.internal.model.ViewStreamModel;
1110
import io.opentelemetry.sdk.metrics.View;
1211
import io.opentelemetry.sdk.metrics.ViewBuilder;
13-
import java.util.List;
1412

1513
final class ViewFactory implements Factory<ViewStreamModel, View> {
1614

@@ -33,9 +31,8 @@ public View create(ViewStreamModel model, DeclarativeConfigContext context) {
3331
}
3432
IncludeExcludeModel attributeKeys = model.getAttributeKeys();
3533
if (attributeKeys != null) {
36-
List<String> included = attributeKeys.getIncluded();
37-
List<String> excluded = attributeKeys.getExcluded();
38-
builder.setAttributeFilter(IncludeExcludePredicate.createExactMatching(included, excluded));
34+
builder.setAttributeFilter(
35+
IncludeExcludeFactory.getInstance().create(attributeKeys, context));
3936
}
4037
if (model.getAggregation() != null) {
4138
builder.setAggregation(

‎sdk-extensions/incubator/src/test/java/io/opentelemetry/sdk/extension/incubator/fileconfig/ComposableRuleBasedSamplerFactoryTest.java‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,11 @@
1313
import static io.opentelemetry.sdk.extension.incubator.fileconfig.ComposableRuleBasedSamplerFactory.DeclarativeConfigSamplingPredicate.toSpanParent;
1414
import static java.util.Collections.emptyList;
1515
import static org.assertj.core.api.Assertions.assertThat;
16+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
1617

1718
import io.opentelemetry.api.common.AttributeKey;
1819
import io.opentelemetry.api.common.Attributes;
20+
import io.opentelemetry.api.incubator.config.DeclarativeConfigException;
1921
import io.opentelemetry.api.trace.Span;
2022
import io.opentelemetry.api.trace.SpanContext;
2123
import io.opentelemetry.api.trace.TraceFlags;
@@ -58,6 +60,76 @@ void create(ExperimentalComposableRuleBasedSamplerModel model, ComposableSampler
5860
assertThat(composableSampler.toString()).isEqualTo(expectedResult.toString());
5961
}
6062

63+
@ParameterizedTest
64+
@MethodSource("createInvalidTestCases")
65+
void createInvalid(ExperimentalComposableRuleBasedSamplerModel model, String expectedMessage) {
66+
assertThatThrownBy(() -> ComposableRuleBasedSamplerFactory.getInstance().create(model, context))
67+
.isInstanceOf(DeclarativeConfigException.class)
68+
.hasMessage(expectedMessage);
69+
}
70+
71+
private static Stream<Arguments> createInvalidTestCases() {
72+
return Stream.of(
73+
Arguments.of(
74+
new ExperimentalComposableRuleBasedSamplerModel()
75+
.withRules(
76+
Collections.singletonList(
77+
new ExperimentalComposableRuleBasedSamplerRuleModel()
78+
.withAttributePatterns(
79+
new ExperimentalComposableRuleBasedSamplerRuleAttributePatternsModel()
80+
.withKey("http.path")
81+
.withIncluded(Collections.emptyList())
82+
.withExcluded(null))
83+
.withSampler(
84+
new ExperimentalComposableSamplerModel()
85+
.withAlwaysOn(
86+
new ExperimentalComposableAlwaysOnSamplerModel())))),
87+
"included must not be empty"),
88+
Arguments.of(
89+
new ExperimentalComposableRuleBasedSamplerModel()
90+
.withRules(
91+
Collections.singletonList(
92+
new ExperimentalComposableRuleBasedSamplerRuleModel()
93+
.withAttributePatterns(
94+
new ExperimentalComposableRuleBasedSamplerRuleAttributePatternsModel()
95+
.withKey("http.path")
96+
.withIncluded(null)
97+
.withExcluded(Collections.emptyList()))
98+
.withSampler(
99+
new ExperimentalComposableSamplerModel()
100+
.withAlwaysOn(
101+
new ExperimentalComposableAlwaysOnSamplerModel())))),
102+
"excluded must not be empty"),
103+
Arguments.of(
104+
new ExperimentalComposableRuleBasedSamplerModel()
105+
.withRules(
106+
Collections.singletonList(
107+
new ExperimentalComposableRuleBasedSamplerRuleModel()
108+
.withAttributeValues(
109+
new ExperimentalComposableRuleBasedSamplerRuleAttributeValuesModel()
110+
.withKey("http.route")
111+
.withValues(null))
112+
.withSampler(
113+
new ExperimentalComposableSamplerModel()
114+
.withAlwaysOn(
115+
new ExperimentalComposableAlwaysOnSamplerModel())))),
116+
".values is required and must be non-empty"),
117+
Arguments.of(
118+
new ExperimentalComposableRuleBasedSamplerModel()
119+
.withRules(
120+
Collections.singletonList(
121+
new ExperimentalComposableRuleBasedSamplerRuleModel()
122+
.withAttributeValues(
123+
new ExperimentalComposableRuleBasedSamplerRuleAttributeValuesModel()
124+
.withKey("http.route")
125+
.withValues(Collections.emptyList()))
126+
.withSampler(
127+
new ExperimentalComposableSamplerModel()
128+
.withAlwaysOn(
129+
new ExperimentalComposableAlwaysOnSamplerModel())))),
130+
".values is required and must be non-empty"));
131+
}
132+
61133
private static Stream<Arguments> createTestCases() {
62134
return Stream.of(
63135
Arguments.of(

‎sdk-extensions/incubator/src/test/java/io/opentelemetry/sdk/extension/incubator/fileconfig/IncludeExcludeFactoryTest.java‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.sdk.extension.incubator.fileconfig;
7+
8+
import static org.assertj.core.api.Assertions.assertThat;
9+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
10+
11+
import io.opentelemetry.api.incubator.config.DeclarativeConfigException;
12+
import io.opentelemetry.common.ComponentLoader;
13+
import io.opentelemetry.sdk.common.internal.IncludeExcludePredicate;
14+
import io.opentelemetry.sdk.extension.incubator.fileconfig.internal.model.IncludeExcludeModel;
15+
import java.util.Arrays;
16+
import java.util.Collections;
17+
import java.util.function.Predicate;
18+
import java.util.stream.Stream;
19+
import org.junit.jupiter.params.ParameterizedTest;
20+
import org.junit.jupiter.params.provider.Arguments;
21+
import org.junit.jupiter.params.provider.MethodSource;
22+
23+
class IncludeExcludeFactoryTest {
24+
25+
private final DeclarativeConfigContext context =
26+
new DeclarativeConfigContext(ComponentLoader.forClassLoader(getClass().getClassLoader()));
27+
28+
@ParameterizedTest
29+
@MethodSource("createArguments")
30+
void create(IncludeExcludeModel model, Predicate<String> expectedPredicate) {
31+
Predicate<String> predicate = IncludeExcludeFactory.getInstance().create(model, context);
32+
assertThat(predicate.toString()).isEqualTo(expectedPredicate.toString());
33+
}
34+
35+
private static Stream<Arguments> createArguments() {
36+
return Stream.of(
37+
// included null, excluded null
38+
Arguments.of(
39+
new IncludeExcludeModel().withIncluded(null).withExcluded(null),
40+
IncludeExcludePredicate.createPatternMatching(null, null)),
41+
// included present, excluded null
42+
Arguments.of(
43+
new IncludeExcludeModel().withIncluded(Arrays.asList("foo", "bar")).withExcluded(null),
44+
IncludeExcludePredicate.createPatternMatching(Arrays.asList("foo", "bar"), null)),
45+
// included null, excluded present
46+
Arguments.of(
47+
new IncludeExcludeModel()
48+
.withIncluded(null)
49+
.withExcluded(Collections.singletonList("baz")),
50+
IncludeExcludePredicate.createPatternMatching(null, Collections.singletonList("baz"))),
51+
// both included and excluded present
52+
Arguments.of(
53+
new IncludeExcludeModel()
54+
.withIncluded(Arrays.asList("foo", "bar"))
55+
.withExcluded(Collections.singletonList("baz")),
56+
IncludeExcludePredicate.createPatternMatching(
57+
Arrays.asList("foo", "bar"), Collections.singletonList("baz"))));
58+
}
59+
60+
@ParameterizedTest
61+
@MethodSource("createInvalidArguments")
62+
void createInvalid(IncludeExcludeModel model, String expectedMessage) {
63+
assertThatThrownBy(() -> IncludeExcludeFactory.getInstance().create(model, context))
64+
.isInstanceOf(DeclarativeConfigException.class)
65+
.hasMessage(expectedMessage);
66+
}
67+
68+
private static Stream<Arguments> createInvalidArguments() {
69+
return Stream.of(
70+
Arguments.of(
71+
new IncludeExcludeModel().withIncluded(Collections.emptyList()).withExcluded(null),
72+
"included must not be empty"),
73+
Arguments.of(
74+
new IncludeExcludeModel().withIncluded(null).withExcluded(Collections.emptyList()),
75+
"excluded must not be empty"));
76+
}
77+
}

‎sdk-extensions/incubator/src/test/java/io/opentelemetry/sdk/extension/incubator/fileconfig/ResourceFactoryTest.java‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -145,11 +145,6 @@ private static Stream<Arguments> createWithDetectorsArgs() {
145145
Collections.singletonList("*o*"),
146146
Collections.singletonList("order"),
147147
Resource.getDefault().toBuilder().put("color", "red").build()),
148-
// empty or missing include should be treated as include all
149-
Arguments.of(
150-
Collections.emptyList(),
151-
Collections.singletonList("order"),
152-
Resource.getDefault().toBuilder().put("color", "red").put("shape", "square").build()),
153148
Arguments.of(
154149
null,
155150
Collections.singletonList("order"),
@@ -191,6 +186,24 @@ private static Stream<Arguments> createInvalidDetectorsArgs() {
191186
new ExperimentalResourceDetectionModel()
192187
.withDetectors(
193188
Collections.singletonList(new ExperimentalResourceDetectorModel()))),
194-
"resource detector must have exactly one entry but has 0"));
189+
"resource detector must have exactly one entry but has 0"),
190+
Arguments.of(
191+
new ResourceModel()
192+
.withDetectionDevelopment(
193+
new ExperimentalResourceDetectionModel()
194+
.withAttributes(
195+
new IncludeExcludeModel()
196+
.withIncluded(Collections.emptyList())
197+
.withExcluded(null))),
198+
"included must not be empty"),
199+
Arguments.of(
200+
new ResourceModel()
201+
.withDetectionDevelopment(
202+
new ExperimentalResourceDetectionModel()
203+
.withAttributes(
204+
new IncludeExcludeModel()
205+
.withIncluded(null)
206+
.withExcluded(Collections.emptyList()))),
207+
"excluded must not be empty"));
195208
}
196209
}

‎sdk-extensions/incubator/src/test/java/io/opentelemetry/sdk/extension/incubator/fileconfig/ViewFactoryTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ void create() {
4242
.setName("name")
4343
.setDescription("description")
4444
.setAttributeFilter(
45-
IncludeExcludePredicate.createExactMatching(
45+
IncludeExcludePredicate.createPatternMatching(
4646
Arrays.asList("foo", "bar"), Collections.singletonList("baz")))
4747
.setAggregation(
4848
Aggregation.explicitBucketHistogram(

0 commit comments

Comments
 (0)