4119 tomas by TomasLongo · Pull Request #2 · KarstenSchnitter/data-prepper

TomasLongo · 2024-10-04T09:05:21Z

No description provided.

(cherry picked from commit 0d45f77) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

(cherry picked from commit f8ac48e) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

(cherry picked from commit ff675dc) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

(cherry picked from commit 43ba7ee) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

(cherry picked from commit 1f90615) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

(cherry picked from commit 2977c1f) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

(cherry picked from commit 473db0e) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

(cherry picked from commit 6ef9b7e) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

(cherry picked from commit 091e9a6) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

(cherry picked from commit a588b09) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

KarstenSchnitter

Thanks for the PR and the effort. Still some minor comments.

KarstenSchnitter · 2024-10-04T10:51:42Z

data-prepper-plugins/otel-logs-source/README.md

+    retry_info:
+      min_delay: 1000ms # defaults to 100ms
+      max_delay: 5s     # defaults to 2s
+


Suggested change

KarstenSchnitter · 2024-10-04T10:52:48Z

data-prepper-plugins/otel-logs-source/README.md


+### Retry Information
+
+Data Prepper gives clients a hint on how long to wait for the next request in case backpressure builds up. The retry information is implemented as exponential backoff, with a max delay of `retry_info.max_delay`.


Suggested change

Data Prepper gives clients a hint on how long to wait for the next request in case backpressure builds up. The retry information is implemented as exponential backoff, with a max delay of `retry_info.max_delay`.

Data Prepper replies with a `RetryInfo` specifying how long to wait for the next request in case backpressure builds up. The retry information is implemented as exponential backoff, with a max delay of `retry_info.max_delay`.

KarstenSchnitter · 2024-10-04T10:55:27Z

...-source/src/main/java/org/opensearch/dataprepper/plugins/source/otellogs/OTelLogsSource.java

-import com.linecorp.armeria.server.encoding.DecodingService;
-import org.opensearch.dataprepper.GrpcRequestExceptionHandler;
-import org.opensearch.dataprepper.plugins.codec.CompressionOption;
-import org.opensearch.dataprepper.plugins.health.HealthGrpcService;
-import org.opensearch.dataprepper.plugins.source.otellogs.certificate.CertificateProviderFactory;
-import com.linecorp.armeria.server.Server;
-import com.linecorp.armeria.server.ServerBuilder;
-import com.linecorp.armeria.server.grpc.GrpcService;
-import com.linecorp.armeria.server.grpc.GrpcServiceBuilder;
-import io.grpc.MethodDescriptor;
-import io.grpc.ServerInterceptor;
-import io.grpc.ServerInterceptors;
-import io.grpc.protobuf.services.ProtoReflectionService;
+import java.io.ByteArrayInputStream;
+import java.nio.charset.StandardCharsets;
+import java.time.Duration;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Executors;


Can you do something to your IDE configuration to avoid reordering of the dependencies?

reverted the import order.

KarstenSchnitter · 2024-10-04T10:58:54Z

...-source/src/main/java/org/opensearch/dataprepper/plugins/source/otellogs/OTelLogsSource.java

+        Duration defaultMinDelay = Duration.ofMillis(100);
+        Duration defaultMaxDelay = Duration.ofMillis(2000);


Provide these values a constants in the class. You can even just define something like this:

// Default RetryInfo with minimum 100ms and maximum 2s private final RetryInfoConfig DEFAULT_RETRY_INFO = new RetryInfoConfig(Duration.ofMillis(100), Duration.ofMillis(2000));

KarstenSchnitter · 2024-10-04T11:00:58Z

.../test/java/org/opensearch/dataprepper/plugins/source/otellogs/OtelLogsSourceConfigTests.java

        settings.put(SSL_KEY_FILE, sslKeyFile);
        settings.put(THREAD_COUNT, threadCount);
        settings.put(MAX_CONNECTION_COUNT, maxConnectionCount);
+        settings.put(OTelLogsSourceConfig.RETRY_INFO, new RetryInfoConfig(Duration.ofMillis(50), Duration.ofMillis(100)));


Can OTelLogsSourceConfig.RETRY_INFO be inlined to RETRY_INFO?

done: Added a static import, if that's what you meant.

KarstenSchnitter · 2024-10-04T11:05:03Z

data-prepper-plugins/otel-metrics-source/README.md


+### Retry Information
+
+Data Prepper gives clients a hint on how long to wait for the next request in case backpressure builds up. The retry information is implemented as exponential backoff, with a max delay of `retry_info.max_delay`.


See comment in logs source README.md

KarstenSchnitter · 2024-10-04T11:05:33Z

...e/src/main/java/org/opensearch/dataprepper/plugins/source/otelmetrics/OTelMetricsSource.java


+    private GrpcExceptionHandlerFunction createGrpExceptionHandler() {
+        Duration defaultMinDelay = Duration.ofMillis(100);
+        Duration defaultMaxDelay = Duration.ofMillis(2000);


See comment in OTelLogsSource.

...java/org/opensearch/dataprepper/plugins/source/otelmetrics/OtelMetricsSourceConfigTests.java

KarstenSchnitter · 2024-10-04T11:06:26Z

data-prepper-plugins/otel-trace-source/README.md

+
+### Retry Information
+
+Data Prepper gives clients a hint on how long to wait for the next request in case backpressure builds up. The retry information is implemented as exponential backoff, with a max delay of `retry_info.max_delay`.


See comment in logs source README.md.

KarstenSchnitter · 2024-10-04T11:06:46Z

...ource/src/main/java/org/opensearch/dataprepper/plugins/source/oteltrace/OTelTraceSource.java

+        Duration defaultMinDelay = Duration.ofMillis(100);
+        Duration defaultMaxDelay = Duration.ofMillis(2000);


See comment in OTelLogsSource.

Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Tomas Longo added 10 commits October 4, 2024 11:01

Make backoff calculation configurable

d6119b6

(cherry picked from commit 0d45f77) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Rename RetryInfo -> RetryInfoConfig

52d2488

(cherry picked from commit f8ac48e) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Create test for back-off calculation

8c667a7

(cherry picked from commit ff675dc) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Return retry info in metrics source

a42ccce

(cherry picked from commit 43ba7ee) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Return retry info in logs source

603f4fb

(cherry picked from commit 1f90615) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Get rid of a few warnings

7c6bb9a

(cherry picked from commit 2977c1f) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Use Duration for delays in RetryInfoConfig

c2bc33f

(cherry picked from commit 473db0e) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Use Duration for delays in RetryInfoConfig for OTelLogsSource

da5b668

(cherry picked from commit 6ef9b7e) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Use Duration for delays in RetryInfoConfig for OTelMetricsSource

9c6d808

(cherry picked from commit 091e9a6) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

Add documentation for retry information

ccbfc9b

(cherry picked from commit a588b09) Signed-off-by: Tomas Longo <tomas.longo@sap.com>

TomasLongo requested a review from KarstenSchnitter as a code owner October 4, 2024 09:05

TomasLongo changed the base branch from 4119-tomas to 4119 October 4, 2024 09:27

KarstenSchnitter requested changes Oct 4, 2024

View reviewed changes

Add review findings

9f3a39d

Signed-off-by: Tomas Longo <tomas.longo@sap.com>

KarstenSchnitter approved these changes Oct 4, 2024

View reviewed changes

KarstenSchnitter merged commit f4b0c60 into KarstenSchnitter:4119 Oct 4, 2024


		### Retry Information

		Data Prepper gives clients a hint on how long to wait for the next request in case backpressure builds up. The retry information is implemented as exponential backoff, with a max delay of `retry_info.max_delay`.

	Data Prepper gives clients a hint on how long to wait for the next request in case backpressure builds up. The retry information is implemented as exponential backoff, with a max delay of `retry_info.max_delay`.
	Data Prepper replies with a `RetryInfo` specifying how long to wait for the next request in case backpressure builds up. The retry information is implemented as exponential backoff, with a max delay of `retry_info.max_delay`.

		Duration defaultMinDelay = Duration.ofMillis(100);
		Duration defaultMaxDelay = Duration.ofMillis(2000);

Conversation

TomasLongo commented Oct 4, 2024

Uh oh!

KarstenSchnitter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i-vim Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

i-vim Oct 4, 2024 •

edited

Loading