Skip to content

Send RetryInfo on OTel Timeouts#4294

Merged
KarstenSchnitter merged 20 commits intoopensearch-project:mainfrom
KarstenSchnitter:4119
Nov 13, 2024
Merged

Send RetryInfo on OTel Timeouts#4294
KarstenSchnitter merged 20 commits intoopensearch-project:mainfrom
KarstenSchnitter:4119

Conversation

@KarstenSchnitter
Copy link
Copy Markdown
Collaborator

@KarstenSchnitter KarstenSchnitter commented Mar 18, 2024 •
edited
Loading

Description

DataPrepper is sending RESOURCE_EXHAUSTED gRPC responses whenever a buffer is full or a circuit breaker is active. These statuses do not contain a retry info. In the OpenTelemetry protocol, this implies a non-retryable error, that will lead to message drops, e.g. in the OTel collector. To apply proper back pressure in these scenarios a retry info is added to the status.

Issues Resolved

Resolves #4119

Check List

  • New functionality includes testing.
  • New functionality has a documentation issue. Please link to it in this PR.
  • 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.

@KarstenSchnitter
Copy link
Copy Markdown
Collaborator Author

KarstenSchnitter commented Mar 18, 2024

@dlvenable this PR shows how to add a RetryInfo. It is lacking a proper determination of a retry delay, which is currently hard-coded to 100ms. I can progress with my proposal from #4119 (comment). It would be nice to get configuration data for an initial delay into GrpcRequestExceptionHandler. Is there an example to go by?

@dlvenable
Copy link
Copy Markdown
Member

dlvenable commented Mar 19, 2024 •
edited
Loading

@dlvenable this PR shows how to add a RetryInfo. It is lacking a proper determination of a retry delay, which is currently hard-coded to 100ms. I can progress with my proposal from #4119 (comment). It would be nice to get configuration data for an initial delay into GrpcRequestExceptionHandler. Is there an example to go by?

@KarstenSchnitter , What do you think about this configuration?

source:
    - otel_traces_source:
         retry_info:
           initial_delay: 100ms
           max_delay: 2s

Here is a code example of a nested configuration:

data-prepper/data-prepper-plugins/s3-source/src/main/java/org/opensearch/dataprepper/plugins/source/s3/S3SourceConfig.java

Lines 45 to 47 in 6a30c6f

@JsonProperty("sqs")
@Valid
private SqsOptions sqsOptions;

The actual implementation is SqsOptions which is another simple POJO class.

@dlvenable
Copy link
Copy Markdown
Member

dlvenable commented Apr 9, 2024

@KarstenSchnitter , What do you have remaining to make this PR ready for review? We did discuss having it be configurable, but anything else to add?

@KarstenSchnitter
Copy link
Copy Markdown
Collaborator Author

KarstenSchnitter commented Apr 11, 2024 •
edited
Loading

I am mostly lacking time to make the required changes 😉:

  • merge last changes on main
  • configuration of the minimal and maximum delay
  • integration test
  • manual test with OTel Collector

@KarstenSchnitter
WIP Send RetryInfo on OTel Timeouts
7dd334b 
DataPrepper is sending `RESOURCE_EXHAUSTED` gRPC responses
whenever a buffer is full or a circuit breaker is active. These statuses do
not contain a retry info. In the OpenTelemetry protocol, this implies a
non-retryable error, that will lead to message drops, e.g. in the OTel
collector. To apply proper back pressure in these scenarios a retry info is
added to the status.

Signed-off-by: Karsten Schnitter <k.schnitter@sap.com>
@KarstenSchnitter
WIP Add RequestInfo Delay Calculator
85a7a18 
Implementation of exponential backoff. Idea is to start with a minimum
delay on the first time-out or circuit breaker activation. If the next such
event happens within twice the last delay after the previous event, double
the delay until a maximum delay is reached. Use the maximum delay from
then on, until a sufficiently long period (maximum delay) without an event
happens. Then the delay is reset to minimum.

TODO: Make minimum and maximum delay configurable.
Signed-off-by: Karsten Schnitter <k.schnitter@sap.com>
Tomas Longo and others added 12 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>
Add review findings
9f3a39d 
Signed-off-by: Tomas Longo <tomas.longo@sap.com>
@KarstenSchnitter
Merge pull request #2 from sternadsoftware/4119-tomas
f4b0c60 
Add RetryInfo Configuration
@KarstenSchnitter
Copy link
Copy Markdown
Collaborator Author

KarstenSchnitter commented Oct 4, 2024

I got help by Tomas Longo, who provided the missing configuration and tests. We also tested, that the RetryInfo is correctly picked up by the OpenTelemetry Collector. With this change Data Prepper exercises back-pressure if the circuit breakers are active.

@KarstenSchnitter KarstenSchnitter marked this pull request as ready for review October 4, 2024 19:02
Tomas Longo and others added 2 commits October 8, 2024 16:01
Add java time module to tests
b3fe06d 
Signed-off-by: Tomas Longo <tomas.longo@sap.com>
@KarstenSchnitter
Merge pull request #3 from sternadsoftware/4119
1232498 
Add java time module to tests
@KarstenSchnitter
4119 Initialize with a buffer to prevent pre-mature backoff calculation
b29246a 
Signed-off-by: Tomas Longo <tomas.longo@sap.com>
@KarstenSchnitter
Copy link
Copy Markdown
Collaborator Author

KarstenSchnitter commented Oct 11, 2024

There was a slight issue with the initialisation of the RetryCalculator, that showed up in the tests. This is now fixed. @dlvenable: Can you take another look at this change?

@KarstenSchnitter KarstenSchnitter changed the title WIP Send RetryInfo on OTel Timeouts Send RetryInfo on OTel Timeouts Oct 18, 2024
dlvenable
dlvenable requested changes Oct 30, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@dlvenable dlvenable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @KarstenSchnitter , this should be a good improvement for OTel clients.

KarstenSchnitter and others added 3 commits November 1, 2024 21:17
@KarstenSchnitter @dlvenable
Apply suggestions from code review
3feda83 
Co-authored-by: David Venable <dlv@amazon.com>
Signed-off-by: Karsten Schnitter <k.schnitter@sap.com>
@KarstenSchnitter @dlvenable
Apply suggestions from code review
9e49340 
Co-authored-by: David Venable <dlv@amazon.com>
Signed-off-by: Karsten Schnitter <k.schnitter@sap.com>
@KarstenSchnitter
Rename Test-Cases to Adhere to Convention
36deb0c 
Signed-off-by: Karsten Schnitter <k.schnitter@sap.com>
@KarstenSchnitter
Copy link
Copy Markdown
Collaborator Author

KarstenSchnitter commented Nov 4, 2024

@dlvenable I renamed the tests. Can you have a look again. I think, that the build failures are caused by different components, not this changeset.

dlvenable
dlvenable approved these changes Nov 12, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@dlvenable dlvenable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @KarstenSchnitter !

@KarstenSchnitter KarstenSchnitter merged commit 2595076 into opensearch-project:main Nov 13, 2024
@KarstenSchnitter KarstenSchnitter deleted the 4119 branch November 13, 2024 15:11
@KarstenSchnitter KarstenSchnitter mentioned this pull request Apr 21, 2025
Merged
4 tasks
@dlvenable dlvenable added this to the v2.11 milestone Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dlvenable dlvenable dlvenable approved these changes

@chenqi0805 chenqi0805 Awaiting requested review from chenqi0805

@engechas engechas Awaiting requested review from engechas engechas is a code owner

@graytaylor0 graytaylor0 Awaiting requested review from graytaylor0 graytaylor0 is a code owner

@dinujoh dinujoh Awaiting requested review from dinujoh dinujoh is a code owner

@kkondaka kkondaka Awaiting requested review from kkondaka kkondaka is a code owner

@oeyh oeyh Awaiting requested review from oeyh oeyh is a code owner

@sb2k16 sb2k16 Awaiting requested review from sb2k16 sb2k16 is a code owner

Assignees

No one assigned

Labels

follow up

Projects

None yet

Milestone

v2.11

Development

Successfully merging this pull request may close these issues.

Improve or Clarify Backpressure on OTel-Grpc-Sources

3 participants

@KarstenSchnitter @dlvenable @sb2k16