Add hasTags() function to dataprepper expressions #2680

kkondaka · 2023-05-11T21:21:14Z

Description

Add hasTags() function to dataprepper expressions.
Data Prepper expressions can now check about presence of tags in the event.
For example an expression may look like
hasTag("tag1") and /status_code == 200
which returns true if the event has a tag with name "tag1" and status_code value is 200.

Resolves #629

Issues Resolved

#629

Check List

[ X] New functionality includes testing.
[ X] New functionality has been documented.
- New functionality has javadoc added
[X ] Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable · 2023-05-11T21:56:39Z

...ssion/src/test/java/org/opensearch/dataprepper/expression/HasTagsExpressionFunctionTest.java

+        tags = new ArrayList<>();
+        Random random = new Random();
+        numTags = random.nextInt(9) + 1;
+        for (int i = 0; i < numTags; i++) {


I think we should control the number of arguments in the testing.

That is, we should test with some specific number of arguments. I'd definitely recommend testing with 1 argument and "more than 1" argument - could be 2, could be 10.

This could be done through @Nested. But, I think this could also be done using @ParameterizedTest where you parameterize the number of tags.

e.g.

@ParameterizedTest @ValueSource(ints = { 1, 2, 10 } void testHasTagsBasic(int numTags /* Also, remove the field used */) { hasTagsExpressionFunction = createObjectUnderTest(); tags = generateTags(numTags); // refactor this code to generate the tags on the fly for (int i = 1; i <= numTags; i++) { assertThat(hasTagsExpressionFunction.evaluate(tags.subList(0, i), testEvent, testFunction), equalTo(true)); } }

This might be improved upon by my more recent suggestion to add EventMetadata::hasTags.

dlvenable · 2023-05-11T22:03:20Z

...xpression/src/main/java/org/opensearch/dataprepper/expression/HasTagsExpressionFunction.java

+        if (args.size() == 0) {
+            throw new RuntimeException("hasTags() takes at least one argument");
+        }
+        Set<String> tags = event.getMetadata().getTags();


I think we may benefit from adding the hasTags() method to the EventMetadata.

This has a couple benefits in my opinion:

It separates the concerns - determining whether an Event has a tag versus interpreting the function arguments.

It creates a function that could end up being useful elsewhere.

You can make the tests for this focus more on the arguments.

In EventMetadata:

boolean hasTags(List<String> tags) { for(String tag : tags) { if(!tags.contains(tag)) { return false; } } return true; }

This code can then be something like:

if(!args.stream() .allMatch(a -> a instanceof String)) { throw new RuntimeException("..."); } final List<String> tags = args.stream() .map(a -> (String) a) .collect(Collectors.toList()); return event.getMetadata().hasTags(tags);

graytaylor0 · 2023-05-11T22:28:37Z

docs/expression_syntax.md

+   - takes at least one argument
+   - all arguments must be of String type
+   - returns true if all arguments are present in the event's tags, returns false otherwise
+   For example, if event has tags "tag1", "tag2", and "tag3", `hasTags("tag1")` or `hasTags("tag1", "tag2")` would return true and `hasTags("tag4")` would return false.


What about hasTags("tag1", "tag4")? False? Please add this as an example as well

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable · 2023-05-11T23:19:20Z

...epper-api/src/test/java/org/opensearch/dataprepper/model/event/DefaultEventMetadataTest.java

-        assertTrue(result.hasTag("tag1"));
-        assertTrue(result.hasTag("tag2"));
-        assertTrue(result.hasTag("tag3"));
+        assertTrue(result.hasTags(List.of("tag1")));


We should have some negative tests cases.

assertFalse(result.hasTag("notPresent"); assertFalse(result.hasTag("notPresent1", "notPresent2"); assertFalse(result.hasTag("tag1", "notPresent");

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

codecov · 2023-05-12T03:30:40Z

Codecov Report

Merging #2680 (0237ddb) into main (a63e5d8) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #2680      +/-   ##
============================================
- Coverage     93.56%   93.54%   -0.02%     
- Complexity     2238     2250      +12     
============================================
  Files           261      262       +1     
  Lines          6275     6291      +16     
  Branches        519      520       +1     
============================================
+ Hits           5871     5885      +14     
- Misses          266      268       +2     
  Partials        138      138

Impacted Files	Coverage Δ
.../dataprepper/model/event/DefaultEventMetadata.java	`91.30% <100.00%> (+0.82%)`	⬆️
...aprepper/expression/HasTagsExpressionFunction.java	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

…literals Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable · 2023-05-12T15:39:57Z

...xpression/src/main/java/org/opensearch/dataprepper/expression/HasTagsExpressionFunction.java

@@ -21,11 +21,14 @@ public Object evaluate(final List<Object> args, Event event, Function<Object, Ob
        if (args.size() == 0) {
            throw new RuntimeException("hasTags() takes at least one argument");
        }
-        if(!args.stream().allMatch(a -> a instanceof String)) {
-            throw new RuntimeException("hasTags() takes only String type arguments");
+        if(!args.stream().allMatch(a -> ((a instanceof String) && (((String)a).length() > 0) && (((String)a).charAt(0) == '"')))) {


I'd probably consider splitting this so that if this error pops up, we can better know which situation it is. But, I don't think this is critical for now.

dlvenable · 2023-05-12T15:40:57Z

...xpression/src/main/java/org/opensearch/dataprepper/expression/HasTagsExpressionFunction.java

        }
        final List<String> tags = args.stream()
-                                    .map(a -> (String) a)
+                                    .map(a -> {


You can split map functions so that you can use the compact lambda form. This is not critical though.

.map(a -> (String)a) .map(str -> str.substring(1, str.length() - 1))

…#2680) * Add hasTags() function to dataprepper expressions Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Addressed review comments Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Updated documentation Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Addressed review comments Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Fixed code coverage build failure Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Modified to make sure that the arguments passed to hasTags is string literals Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> --------- Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> Signed-off-by: rajeshLovesToCode <rajesh.dharamdasani3021@gmail.com>

…-project#2680)" This reverts commit 76c1753.

Add hasTags() function to dataprepper expressions

6d188e3

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

kkondaka requested review from chenqi0805, engechas, graytaylor0, dinujoh, cmanning09, asifsmohammed, dlvenable and oeyh as code owners May 11, 2023 21:21

dlvenable requested changes May 11, 2023

View reviewed changes

graytaylor0 reviewed May 11, 2023

View reviewed changes

kkondaka added 2 commits May 11, 2023 22:33

Addressed review comments

cc7d278

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

Updated documentation

a42d00f

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable requested changes May 11, 2023

View reviewed changes

kkondaka added 2 commits May 12, 2023 00:56

Addressed review comments

fe2b5ba

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

Fixed code coverage build failure

f57ecd6

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

Modified to make sure that the arguments passed to hasTags is string …

0237ddb

…literals Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable approved these changes May 12, 2023

View reviewed changes

graytaylor0 approved these changes May 12, 2023

View reviewed changes

kkondaka merged commit ee43a60 into opensearch-project:main May 12, 2023
26 checks passed

rajeshLovesToCode added a commit to rajeshLovesToCode/data-prepper that referenced this pull request May 18, 2023

Revert "Add hasTags() function to dataprepper expressions (opensearch…

ca8520b

…-project#2680)" This reverts commit 76c1753.

rajeshLovesToCode added a commit to rajeshLovesToCode/data-prepper that referenced this pull request May 18, 2023

Revert "Add hasTags() function to dataprepper expressions (opensearch…

2fbbef9

…-project#2680)" This reverts commit 76c1753.

kkondaka mentioned this pull request Jun 5, 2023

Tagging Events in Data Prepper #629

Closed

kkondaka deleted the has-tags-func branch July 13, 2023 04:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hasTags() function to dataprepper expressions #2680

Add hasTags() function to dataprepper expressions #2680

kkondaka commented May 11, 2023

dlvenable May 11, 2023

dlvenable May 11, 2023

dlvenable May 11, 2023 •

edited

graytaylor0 May 11, 2023

dlvenable May 11, 2023

codecov bot commented May 12, 2023 •

edited

dlvenable May 12, 2023

dlvenable May 12, 2023

Add hasTags() function to dataprepper expressions #2680

Add hasTags() function to dataprepper expressions #2680

Conversation

kkondaka commented May 11, 2023

Description

Issues Resolved

Check List

dlvenable May 11, 2023

Choose a reason for hiding this comment

dlvenable May 11, 2023

Choose a reason for hiding this comment

dlvenable May 11, 2023 • edited

Choose a reason for hiding this comment

graytaylor0 May 11, 2023

Choose a reason for hiding this comment

dlvenable May 11, 2023

Choose a reason for hiding this comment

codecov bot commented May 12, 2023 • edited

Codecov Report

dlvenable May 12, 2023

Choose a reason for hiding this comment

dlvenable May 12, 2023

Choose a reason for hiding this comment

dlvenable May 11, 2023 •

edited

codecov bot commented May 12, 2023 •

edited