MAINT: add OTelTraceGroupProcessor from trace ingestion migration branch #1224

chenqi0805 · 2022-03-18T18:39:40Z

Signed-off-by: Qi Chen [email protected]

Description

This PR adds in otel-trace-group-processor which is otel-trace-group-prepper in https://github.com/opensearch-project/data-prepper/tree/maint/546-migrate-trace-analytics-to-event-model.

Beside renaming plugins, some cleanups are essential:

README: prepper -> processor
package name in new modules: prepper -> processor
implement interface: prepper -> processor

Issues Resolved

Contributes to #1158

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: Qi Chen <[email protected]>

codecov-commenter · 2022-03-18T18:48:10Z

Codecov Report

Merging #1224 (fd73be7) into main (c684f7c) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #1224   +/-   ##
=========================================
  Coverage     93.79%   93.79%           
  Complexity     1104     1104           
=========================================
  Files           153      153           
  Lines          3064     3064           
  Branches        257      257           
=========================================
  Hits           2874     2874           
  Misses          138      138           
  Partials         52       52

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 c684f7c...fd73be7. Read the comment docs.

Signed-off-by: Qi Chen <[email protected]>

cmanning09

Thanks for migrating to the processor interface. I have a comment for updating the documentation.

cmanning09 · 2022-03-23T15:51:38Z

data-prepper-plugins/otel-trace-group-processor/README.md

@@ -0,0 +1,67 @@
+# OTel Trace Group Processor
+
+This is a processor that fills in the missing trace group related fields in the collection of [Span](../../data-prepper-api/src/main/java/com/amazon/dataprepper/model/trace/Span.java) records output by [otel-trace-raw-processor](../otel-trace-raw-processor) and then convert them back into a new collection of string records.


... and then convert them back into a new collection of string records. <- This is no longer accurate and we should be able to remove it.

Great catch!

sshivanii · 2022-03-23T16:12:52Z

.../amazon/dataprepper/plugins/processor/oteltracegroup/OTelTraceGroupProcessorConfigTests.java

+        try (MockedStatic<ConnectionConfiguration> connectionConfigurationMockedStatic = Mockito.mockStatic(ConnectionConfiguration.class)) {
+            connectionConfigurationMockedStatic.when(() -> ConnectionConfiguration.readConnectionConfiguration(any(PluginSetting.class)))
+                    .thenReturn(connectionConfigurationMock);
+            PluginSetting testPluginSetting = new PluginSetting("otel_trace_group_prepper", new HashMap<>());


Please update this to new PluginSetting("otel_trace_group_processor",, new HashMap<>());

Great catch!

sshivanii · 2022-03-23T16:23:04Z

...va/com/amazon/dataprepper/plugins/processor/oteltracegroup/OTelTraceGroupProcessorTests.java

+
+    @Test
+    public void testTraceGroupFillFailDueToFailedRequest() throws IOException {
+        // Arrange


I like how you've separated the functionality within each test case!

Signed-off-by: Qi Chen <[email protected]>

dlvenable · 2022-03-28T18:56:27Z

data-prepper-plugins/otel-trace-group-processor/README.md

+pipeline:
+  ...
+  processor:
+    - otel-trace-group-processor:


This should be using underscores, right?

dlvenable · 2022-03-28T18:56:37Z

data-prepper-plugins/otel-trace-group-processor/README.md

+pipeline:
+  ...
+  processor:
+    - otel-trace-group:


This should be using underscores, right?

dlvenable · 2022-03-28T18:57:46Z

data-prepper-plugins/otel-trace-group-processor/README.md

+pipeline:
+  ...
+  processor:
+    - otel-trace-group-processor:


Also, there is no _processor suffix, right?

Thanks for the catch.

dlvenable · 2022-03-28T18:58:39Z

data-prepper-plugins/otel-trace-group-processor/README.md

@@ -0,0 +1,67 @@
+# OTel Trace Group Processor
+
+This is a processor that fills in the missing trace group related fields in the collection of [Span](../../data-prepper-api/src/main/java/com/amazon/dataprepper/model/trace/Span.java) records output by [otel-trace-raw-processor](../otel-trace-raw-processor).


The name here also should use underscores.

dlvenable · 2022-03-28T19:01:15Z

...in/java/com/amazon/dataprepper/plugins/processor/oteltracegroup/OTelTraceGroupProcessor.java

+            final String traceId = traceIdDocField.getValue();
+            final String traceGroupName = traceGroupNameDocField.getValue();
+            // Restore trailing zeros for thousand, e.g. 2020-08-20T05:40:46.0895568Z -> 2020-08-20T05:40:46.089556800Z
+            final String traceGroupEndTime = Instant.parse(traceGroupEndTimeDocField.getValue()).toString();


Rather than add an inline comment (which is prone to being lost over time), I suggest you make this a function.

Perhaps:

/** * Restores trailing zeros for thousand, e.g. 2020-08-20T05:40:46.0895568Z -> 2020-08-20T05:40:46.089556800Z */ private String normalizeDateTime(String dateTimeString) { return Instant.parse(dateTimeString).toString(); }

Signed-off-by: Qi Chen <[email protected]>

ADD: otel-trace-group-processor

2fbb9bd

Signed-off-by: Qi Chen <[email protected]>

chenqi0805 requested a review from a team as a code owner March 18, 2022 18:39

MAINT: update header

fd73be7

Signed-off-by: Qi Chen <[email protected]>

MAINT: migrate to processor interface

ebe87e5

Signed-off-by: Qi Chen <[email protected]>

cmanning09 reviewed Mar 23, 2022

View reviewed changes

sshivanii reviewed Mar 23, 2022

View reviewed changes

MAINT: README and renaming plugin

878c34d

Signed-off-by: Qi Chen <[email protected]>

chenqi0805 requested review from cmanning09 and sshivanii March 23, 2022 20:28

chenqi0805 mentioned this pull request Mar 25, 2022

Provide a migration path to using Events for Trace data #1158

Closed

chenqi0805 requested a review from a team March 28, 2022 14:33

cmanning09 previously approved these changes Mar 28, 2022

View reviewed changes

dlvenable requested changes Mar 28, 2022

View reviewed changes

DOC: fix plugin names in README

6a28e1c

Signed-off-by: Qi Chen <[email protected]>

chenqi0805 dismissed cmanning09’s stale review via 6a28e1c March 28, 2022 19:35

chenqi0805 requested review from dlvenable and cmanning09 March 28, 2022 19:35

REF: normalizeDateTime

d70ab24

Signed-off-by: Qi Chen <[email protected]>

cmanning09 approved these changes Mar 30, 2022

View reviewed changes

sshivanii approved these changes Mar 30, 2022

View reviewed changes

chenqi0805 requested review from dlvenable and removed request for dlvenable March 30, 2022 20:47

dlvenable approved these changes Mar 30, 2022

View reviewed changes

chenqi0805 merged commit 46b205a into opensearch-project:main Mar 30, 2022

chenqi0805 deleted the maint/add-otel-trace-group-processor branch March 30, 2022 22:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT: add OTelTraceGroupProcessor from trace ingestion migration branch #1224

MAINT: add OTelTraceGroupProcessor from trace ingestion migration branch #1224

chenqi0805 commented Mar 18, 2022 •

edited

Loading

codecov-commenter commented Mar 18, 2022 •

edited

Loading

cmanning09 left a comment

cmanning09 Mar 23, 2022

chenqi0805 Mar 23, 2022

sshivanii Mar 23, 2022

chenqi0805 Mar 23, 2022

sshivanii Mar 23, 2022

dlvenable Mar 28, 2022

dlvenable Mar 28, 2022

dlvenable Mar 28, 2022

chenqi0805 Mar 28, 2022

dlvenable Mar 28, 2022

dlvenable Mar 28, 2022

		@@ -0,0 +1,67 @@
		# OTel Trace Group Processor

		This is a processor that fills in the missing trace group related fields in the collection of [Span](../../data-prepper-api/src/main/java/com/amazon/dataprepper/model/trace/Span.java) records output by [otel-trace-raw-processor](../otel-trace-raw-processor) and then convert them back into a new collection of string records.

MAINT: add OTelTraceGroupProcessor from trace ingestion migration branch #1224

MAINT: add OTelTraceGroupProcessor from trace ingestion migration branch #1224

Conversation

chenqi0805 commented Mar 18, 2022 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Mar 18, 2022 • edited Loading

Codecov Report

cmanning09 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

chenqi0805 commented Mar 18, 2022 •

edited

Loading

codecov-commenter commented Mar 18, 2022 •

edited

Loading