OpenSearch Index DateTime Pattern conversion #1045

sshivanii · 2022-02-16T06:40:11Z

Description

Adds a custom AttributeMapper to converts year, year-of-era, week-year patterns in index from Joda to Java-time:
Removes + from DateTime patterns in Logstash config
Adds test cases

Issues Resolved

Resolves #854

Check List

New functionality includes testing.
New functionality has been documented.
- New functionality has javadoc added
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: Shivani Shukla <[email protected]>

dlvenable

Thanks, this loos good. I made a few suggestions that I think will help improve the code.

dlvenable · 2022-02-16T15:38:13Z

.../main/java/org/opensearch/dataprepper/logstash/mapping/OpenSearchPluginAttributesMapper.java

+    private String convertIndexDateTimePattern(LogstashAttribute logstashAttribute) {
+
+        final String logstashIndexAttributeValue = logstashAttribute.getAttributeValue().getValue().toString();
+        final Pattern pattern = Pattern.compile(REGEX_EXTRACTING_DATETIME_PATTERN);


This pattern can be compiled in the default constructor. Then you can make pattern a field and re-use it each time. It won't have a huge impact, but could help some if there are two or more elasticsearch outputs.

Also, a better name might be dateTimePattern to be clear what the pattern is covering.

That's a good suggestion I can look into. For the name, later on line 52 I'm making a dateTimePattern string as: final String dateTimePattern = dateTimePatternMatcher.group(0);

Yea, pattern means two things here: 1) regex and 2) a date-time pattern.

Maybe you can rename it to dateTimeRegexPattern to be clear in what it provides.

.../main/java/org/opensearch/dataprepper/logstash/mapping/OpenSearchPluginAttributesMapper.java

dlvenable · 2022-02-16T15:40:44Z

.../main/java/org/opensearch/dataprepper/logstash/mapping/OpenSearchPluginAttributesMapper.java

+
+class OpenSearchPluginAttributesMapper extends AbstractLogstashPluginAttributesMapper {
+    protected static final String LOGSTASH_OPENSEARCH_INDEX_ATTRIBUTE_NAME = "index";
+    protected static final String REGEX_EXTRACTING_DATETIME_PATTERN = "%\\{(.*?)\\}";


Should the + be in the pattern?

If it is not a date-time pattern in the Logstash configuration, this would still match. Then we'd make the conversions which are not quite accurate.

That's a good point, I will add the + in the Regex.

dlvenable · 2022-02-16T15:42:42Z

...t/java/org/opensearch/dataprepper/logstash/mapping/OpenSearchPluginAttributesMapperTest.java

+
+        assertThat(pluginSettings, notNullValue());
+        assertThat(pluginSettings.size(), equalTo(1));
+        assertThat(pluginSettings, hasKey(dataPrepperIndexAttribute));


Checking for the key is good. Should you also verify the value after this line?

assertThat(pluginSettings.get(dataPrepperIndexAttribute), equalTo(value));

Will add, I missed it in this test, thanks for catching this.

dlvenable · 2022-02-16T15:43:07Z

...t/java/org/opensearch/dataprepper/logstash/mapping/OpenSearchPluginAttributesMapperTest.java

+
+    @Test
+    void convert_logstash_index_date_time_pattern() {
+        final String dataPrepperIndexAttribute = "index";


You can make this a constant in the class and use it elsewhere. It will make the code more readable since it will be clear that it is a constant.

dlvenable · 2022-02-16T15:49:00Z

...t/java/org/opensearch/dataprepper/logstash/mapping/OpenSearchPluginAttributesMapperTest.java

+    }
+
+    @Test
+    void convert_logstashIndexPattern_joda_year_to_dataPrepperIndexPattern_java_year() {


I think that this test and the next three are the same except for the input and output. You can make this a JUnit 5 parameterized test and this would be clearer to use.

Here is somewhat of a starting point for it.

static class JodaToJava8IndicesArgumentsProvider implements ArgumentsProvider { @Override public Stream<? extends Arguments> provideArguments(final ExtensionContext extensionContext) { return Stream.of( Arguments.arguments("logstash-%{+yyyy.MM.dd}", "logstash-%{uuuu.MM.dd}"), Arguments.arguments("logstash-index-%{+YYYY.MM.dd}", "logstash-index-%{yyyy.MM.dd}"), // repeat for each ); } }

You can see this in use at:

https://github.com/opensearch-project/data-prepper/blob/main/data-prepper-logstash-configuration/src/test/java/org/opensearch/dataprepper/logstash/LogstashConfigConverterIT.java#L52-L54

I had thought of parameterized tests but went ahead with separate tests to clarify each input and output we're expecting. It does seem redundant though so I will look into updating these.

dlvenable · 2022-02-16T16:44:48Z

...t/java/org/opensearch/dataprepper/logstash/mapping/OpenSearchPluginAttributesMapperTest.java

+    }
+
+    @Test
+    void convert_logstash_index_date_time_pattern() {


Also, I think that this test could be part of the parameterized tests as well.

cmanning09 · 2022-02-16T15:18:14Z

.../main/java/org/opensearch/dataprepper/logstash/mapping/OpenSearchPluginAttributesMapper.java

+    protected static final String REGEX_EXTRACTING_DATETIME_PATTERN = "%\\{(.*?)\\}";
+
+    @Override
+    protected void mapCustomAttributes(List<LogstashAttribute> logstashAttributes, LogstashAttributesMappings logstashAttributesMappings, Map<String, Object> pluginSettings) {


All parameters should be marked as final. Please implement elsewhere.

cmanning09 · 2022-02-16T15:43:37Z

...t/java/org/opensearch/dataprepper/logstash/mapping/OpenSearchPluginAttributesMapperTest.java

+import java.util.Collections;
+import java.util.Map;
+
+class OpenSearchPluginAttributesMapperTest {


Let's add a test for when there is no pattern and the index is just "logstash". This should catch a bug with the current implementation.

cmanning09 · 2022-02-16T15:49:44Z

.../main/java/org/opensearch/dataprepper/logstash/mapping/OpenSearchPluginAttributesMapper.java

+        final LogstashAttribute logstashIndexAttribute = logstashAttributes.stream()
+                .filter(a -> a.getAttributeName().equals(LOGSTASH_OPENSEARCH_INDEX_ATTRIBUTE_NAME))
+                .findFirst()
+                .orElseThrow(() -> new LogstashConfigurationException("Index attribute not found"));


The elasticsearch output plugin and the opensearch output plugin both have default index patterns. We should handle those scenarios instead of throwing an exception.

@sshivanii , The AbstractLogstashPluginAttributesMapper class will call mapCustomAttributes whether or not the index is defined. Thus, this exception is invalid as @cmanning09 pointed out.

If the index is not supplied, then there is no custom attribute to process here. I'd recommend changing orElseThrow to ifPresent and then move lines 39 and 41 in there.

Also, this is a good unit test case. Provide an empty logstashAttributes as mocked input. Then verify that the PluginSettings is empty - it should have no index key.

I see, looks like I did not update this in the new revision, will do now. Thanks @cmanning09 @dlvenable

@sshivanii, the current approach is not handling the default values for the elasticsearch output plugin or the opensearch output plugin. We need to support a migration of these indices.

Regarding the opensearch output plugin: we need to dive deep here to understand what the default values our. The documentation is not clear at this time.

@cmanning09 Regarding the default values for index in Logstash ES output plugin, it's dependent on the ecs_compatibility attribute which we do not support in our sink plugin so it will not be added to the converted .yaml file. Since we don't support ecs compatibility, I believe these default values should not be added.

I dug deeper into the OpenSearch code to find they only have a default index depending on ecs being enabled/disabled. Otherwise they throw an exception Unsupported ECS compatibility
https://github.com/opensearch-project/logstash-output-opensearch/blob/main/lib/logstash/outputs/opensearch.rb#L417

I'd like to suggest that when the index name is not supplied, Data Prepper uses the index name for when ECS is disabled. ECS compatibility is a broader topic than this PR is covering, and I don't think it should hinder this PR.

Thus, if the index is not present, the converter can just set the index to logstash-%{uuuu.MM.dd} (assuming I converted that string correctly). The current two tests which have no index value would both need to change to verify this new behavior.

Also, please verify that this is also the default of the amazon_es plugin.

I think the best solution here is to include a defaultAttributes property to the Logstash configuration mapping YAML. Other plugins may need to have default values where the Logstash default is different from the Data Prepper default.

As this problem exists in the current converter and this change introduces no regressions, I'd rather we wrap up this PR and support this more generically in a second PR.

Checked the default index attribute value in amazon_es plugin and it is logstash-%{+YYYY.MM.dd} so the converted value will be logstash-%{yyyy.MM.dd}. This is different from the elasticsearch output plugin default, and the plugin name is not known at this time.
Reference: https://github.com/awslabs/logstash-output-amazon_es/blob/master/lib/logstash/outputs/amazon_es/common_configs.rb#L17

cmanning09 · 2022-02-16T16:43:14Z

.../main/java/org/opensearch/dataprepper/logstash/mapping/OpenSearchPluginAttributesMapper.java

+                if (character == '+') {
+                    continue;
+                } else if (character == 'y') {
+                    updatedDateTimePattern.append('u');
+                } else if (character == 'Y') {
+                    updatedDateTimePattern.append('y');
+                } else if (character == 'x') {
+                    updatedDateTimePattern.append('Y');
+                } else {
+                    updatedDateTimePattern.append(character);
+                }


I think we can simplify this logic by using a map. It is more maintainable and reduces the number of conditional checks required.

We can declare a static map for this class

private static Map<Character, Character> PATTERN_CONVERTER_MAP; static { PATTERN_CONVERTER_MAP = new HashMap<>(); PATTERN_CONVERTER_MAP.put('y', 'u'); PATTERN_CONVERTER_MAP.put('Y', 'y'); PATTERN_CONVERTER_MAP.put('x', 'Y'); }

And then retrieve the converted characters here:

if (character != '+') { final char convertedChar = PATTERN_CONVERTER_MAP.getOrDefault(character, character); updatedDateTimePattern.append(convertedChar); }

cmanning09 · 2022-02-16T17:02:08Z

.../main/java/org/opensearch/dataprepper/logstash/mapping/OpenSearchPluginAttributesMapper.java

+        final StringBuilder updatedDateTimePattern = new StringBuilder();
+        String updatedIndexAttributeValue = "";


As a best practice, variables should be minimally scoped to the block they are used and we should avoid re-assignment where possible. We can achieve this by leveraging an additional private method to handle the match and conversion part.

return dateTimePatternMatcher.find() ? matchAndConvertDateTimePattern(dateTimePatternMatcher, logstashIndexAttributeValue) : logstashIndexAttributeValue;

New method:

private String matchAndConvertDateTimePattern(final Matcher dateTimePatternMatcher, final String logstashIndexAttributeValue) { final String dateTimePattern = dateTimePatternMatcher.group(0); final StringBuilder updatedDateTimePattern = new StringBuilder(); for (final char character: dateTimePattern.toCharArray()) { ... // conversion logic from below. } return logstashIndexAttributeValue.replace(dateTimePattern, updatedDateTimePattern); }

The current implementation also has a bug when a pattern is not provided because we are assigning the updatedIndexAttributeValue to "" and returning "" if a pattern does not exist.

Thanks for catching this. It does substitute "" for plain Strings index names. I will update this and add a test for this usecase.

Signed-off-by: Shivani Shukla <[email protected]>

codecov-commenter · 2022-02-17T22:53:30Z

Codecov Report

Merging #1045 (4f504aa) into main (e5434f6) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #1045      +/-   ##
============================================
- Coverage     91.36%   91.32%   -0.05%     
  Complexity      683      683              
============================================
  Files            84       84              
  Lines          1993     1994       +1     
  Branches        168      168              
============================================
  Hits           1821     1821              
  Misses          132      132              
- Partials         40       41       +1

Impacted Files	Coverage Δ
...m/amazon/dataprepper/model/event/JacksonEvent.java	`98.41% <0.00%> (-0.79%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5434f6...4f504aa. Read the comment docs.

dlvenable · 2022-02-18T16:43:16Z

.../main/java/org/opensearch/dataprepper/logstash/mapping/OpenSearchPluginAttributesMapper.java

+        final LogstashAttribute logstashIndexAttribute = logstashAttributes.stream()
+                .filter(a -> a.getAttributeName().equals(LOGSTASH_OPENSEARCH_INDEX_ATTRIBUTE_NAME))
+                .findFirst()
+                .orElseThrow(() -> new LogstashConfigurationException("Index attribute not found"));


@sshivanii , The AbstractLogstashPluginAttributesMapper class will call mapCustomAttributes whether or not the index is defined. Thus, this exception is invalid as @cmanning09 pointed out.

If the index is not supplied, then there is no custom attribute to process here. I'd recommend changing orElseThrow to ifPresent and then move lines 39 and 41 in there.

Also, this is a good unit test case. Provide an empty logstashAttributes as mocked input. Then verify that the PluginSettings is empty - it should have no index key.

...ation/src/main/resources/org/opensearch/dataprepper/logstash/mapping/opensearch.mapping.yaml

Signed-off-by: Shivani Shukla <[email protected]>

dlvenable · 2022-02-18T21:48:15Z

...t/java/org/opensearch/dataprepper/logstash/mapping/OpenSearchPluginAttributesMapperTest.java

+        final LogstashAttribute logstashAttribute = mock(LogstashAttribute.class);
+        final LogstashAttributeValue logstashAttributeValue = mock(LogstashAttributeValue.class);
+
+        when(logstashAttribute.getAttributeName()).thenReturn("");


This test is testing what happens when index is null. This is a good thing to test for sure.

But, these four lines are confusing. At first glance it looks like you are testing what happens when index is empty (""). These four lines are actually unnecessary and misleading.

We should also have a test for when the index is an empty string (""). You can test this using createLogstashIndexAttribute("") and don't need to copy these four lines around for it.

Understood, added two testcases separating the two scenarios. Thanks for pointing it out.

Thanks. Please change these values from empty strings to random strings. It will make the intent clearer.

Signed-off-by: Shivani Shukla <[email protected]>

dlvenable · 2022-02-18T22:13:26Z

...t/java/org/opensearch/dataprepper/logstash/mapping/OpenSearchPluginAttributesMapperTest.java

+        final Map<String, Object> pluginSettings = createObjectUnderTest()
+                .mapAttributes(Collections.singletonList(logstashAttribute), logstashAttributesMappings);
+
+        assertThat(pluginSettings.size(), equalTo(1));


I don't understand these assertions. There is one plugin setting here. But, it is not the index. What is it?

pluginSettings is creating this: pluginSettings: {null=} where AttributeName is still index since that is a constant but AttributeValue is "" so it's empty.

@dlvenable
I've further clarified the test cases, I hope it's more clear now.

The createLogstashIndexAttribute(final String indexAttributeValue) method is setting a constant "index" for logstashAttribute.getAttributeName() and takes in param for indexAttributeValue

So I've now mocked the logstashAttribute and logstashAttributeValue to be "" in the test case and asserted pluginSettings is empty.

Signed-off-by: Shivani Shukla <[email protected]>

dlvenable

I believe this PR handles the current tasking and is ready to be merged. Please create a new GitHub issue for handling the default values for OpenSearch, Elasticsearch, and amazon_es index names.

cmanning09

@sshivanii, please create an issue to track the delayed work for default values.

sshivanii · 2022-02-22T16:25:41Z

@sshivanii, please create an issue to track the delayed work for default values.

Here's the issue for adding default values. #1080

sshivanii added 3 commits February 15, 2022 23:19

Updated OpenSearch mapping file

4094330

Signed-off-by: Shivani Shukla <[email protected]>

Added AttributesMapper for OpenSearchPlugin index

978af17

Signed-off-by: Shivani Shukla <[email protected]>

Added test cases to convert dateTime patterns

57b57ef

Signed-off-by: Shivani Shukla <[email protected]>

sshivanii requested a review from a team as a code owner February 16, 2022 06:40

dlvenable requested changes Feb 16, 2022

View reviewed changes

dlvenable reviewed Feb 16, 2022

View reviewed changes

cmanning09 reviewed Feb 16, 2022

View reviewed changes

sshivanii added 3 commits February 17, 2022 01:48

Refactored match and convert logic

c8f5bbf

Signed-off-by: Shivani Shukla <[email protected]>

Fixed failed build due to extra space

abbff50

Signed-off-by: Shivani Shukla <[email protected]>

Updated to Paramterized tests

dcf6b39

Signed-off-by: Shivani Shukla <[email protected]>

sshivanii requested review from dlvenable and cmanning09 February 17, 2022 22:46

dlvenable requested changes Feb 18, 2022

View reviewed changes

dlvenable reviewed Feb 18, 2022

View reviewed changes

...ation/src/main/resources/org/opensearch/dataprepper/logstash/mapping/opensearch.mapping.yaml Show resolved Hide resolved

sshivanii added 2 commits February 18, 2022 15:21

Updated mapAttributes when no index attribute is present, added testcase

3c1e506

Signed-off-by: Shivani Shukla <[email protected]>

Added Mapper class in elasticsearch and amazon_es mapping files

3c106b4

Signed-off-by: Shivani Shukla <[email protected]>

sshivanii requested review from dlvenable and cmanning09 and removed request for cmanning09 February 18, 2022 21:24

dlvenable requested changes Feb 18, 2022

View reviewed changes

Clarified test cases for missing and empty Index Attribute

04f92d6

Signed-off-by: Shivani Shukla <[email protected]>

dlvenable reviewed Feb 18, 2022

View reviewed changes

sshivanii added 2 commits February 18, 2022 16:38

Clarify test case

4e92498

Signed-off-by: Shivani Shukla <[email protected]>

Renamed index attribute value

4f504aa

Signed-off-by: Shivani Shukla <[email protected]>

sshivanii requested a review from dlvenable February 20, 2022 22:06

Address feedback

4c82460

Signed-off-by: Shivani Shukla <[email protected]>

dlvenable approved these changes Feb 22, 2022

View reviewed changes

cmanning09 approved these changes Feb 22, 2022

View reviewed changes

sshivanii merged commit d775287 into opensearch-project:main Feb 22, 2022

		final StringBuilder updatedDateTimePattern = new StringBuilder();
		String updatedIndexAttributeValue = "";

OpenSearch Index DateTime Pattern conversion #1045

OpenSearch Index DateTime Pattern conversion #1045

Conversation

sshivanii commented Feb 16, 2022 • edited Loading

Description

Issues Resolved

Check List

dlvenable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlvenable Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlvenable Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

sshivanii Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 17, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlvenable left a comment

Choose a reason for hiding this comment

cmanning09 left a comment

Choose a reason for hiding this comment

sshivanii commented Feb 22, 2022

sshivanii commented Feb 16, 2022 •

edited

Loading

dlvenable Feb 21, 2022 •

edited

Loading

dlvenable Feb 21, 2022 •

edited

Loading

sshivanii Feb 21, 2022 •

edited

Loading

codecov-commenter commented Feb 17, 2022 •

edited

Loading