Enforce IncludedExcludeModel .included and .excluded are not empty#8266
Enforce IncludedExcludeModel .included and .excluded are not empty#8266jack-berg wants to merge 3 commits intoopen-telemetry:mainfrom
Conversation
| import java.util.List; | ||
| import java.util.function.Predicate; | ||
|
|
||
| final class IncludeExcludeFactory implements Factory<IncludeExcludeModel, Predicate<String>> { |
There was a problem hiding this comment.
Since IncludeExcludeFactory is a recurring concept in the config schema, create a dedicated factory which consistently enforces the constraints
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8266 +/- ##
============================================
- Coverage 90.29% 90.28% -0.02%
- Complexity 7656 7667 +11
============================================
Files 844 845 +1
Lines 23071 23088 +17
Branches 2311 2318 +7
============================================
+ Hits 20832 20845 +13
- Misses 1521 1523 +2
- Partials 718 720 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| throw new DeclarativeConfigException(".included must not be empty"); | ||
| } | ||
| List<String> excluded = attributePatternsModel.getExcluded(); | ||
| if (excluded != null && excluded.isEmpty()) { | ||
| throw new DeclarativeConfigException(".excluded must not be empty"); |
There was a problem hiding this comment.
Is the . intentional here ? Can we provide parrent attribute name that helps the user to understand where the error is ? Is there any particular reason not to do the same with prometheus ?
There was a problem hiding this comment.
Is the . intentional here
It was intentional because I thought that was the prevailing pattern but looking at other validation examples I was wrong. Will fix.
Can we provide parrent attribute name that helps the user to understand where the error is ?
I want to solve this holistically in #7949. Currently, we lack the context to have truly useful error messages. Sure we could include the type of the immediate parent, but that's a limited solution.
| import java.util.List; | ||
| import java.util.function.Predicate; | ||
|
|
||
| final class IncludeExcludeFactory implements Factory<IncludeExcludeModel, Predicate<String>> { |
There was a problem hiding this comment.
(minor/style) instead of having a dedicated factory class maybe we could have used a static factory method on IncludeExcludePredicate to keep things simpler as the factory is purely static here and has no state.
There was a problem hiding this comment.
This factory pattern is employed widely in declarative config with the general pattern being one factory per model, with each factory responsible for translating an instance of the model to an instance of a corresponding SDK component. They're all stateless, but having singleton instances is still useful because it forces consistency in the pattern.
Can't move to IncludeExcludePredicate because the model class isn't there. Could have a static method in IncludeExcludePredicate which accepts @Nullable List<String> included and @Nullable List<String> excluded parameters, but DeclarativeConfigException isn't available so would need to wrap the call in a try/catch and map the IllegalArgumentException to DeclarativeConfigException.
2 participants
Followup from #8185 (comment)
See constraints from the schema here: https://github.com/open-telemetry/opentelemetry-configuration/blob/main/schema-docs.md#includeexclude
cc @SylvainJuge