Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add base class for implementing retry strategies in otlp exporter #4872

Merged

Conversation

vishweshbankwar
Copy link
Member

@vishweshbankwar vishweshbankwar commented Sep 21, 2023

Fixes #
Design discussion issue #

Changes

This PR contains a base TransmissionHandler class which can be used to implement retry strategies such as in memory and persistent storage. This is intended to be used only by the internal implementations of retry strategies that will be offered in OTLP exporter.

Example of in-memory retry using this base

internal class OtlpExporterRetryTransmissionHandler<T> : OtlpExporterTransmissionHandler<T>
{
    protected override bool OnSubmitRequestExceptionThrown(T request, Exception exception)
    {
        var retryAttemptCount = 0;

        while (this.ShouldRetryRequest(request, exception, retryAttemptCount++, out var sleepDuration))
        {
            if (sleepDuration > TimeSpan.Zero)
            {
                Thread.Sleep(sleepDuration);
            }

            if (this.RetryRequest(request, out exception))
            {
                return true;
            }
        }

        return this.OnHandleDroppedRequest(request);
    }

    protected virtual bool ShouldRetryRequest(T request, Exception exception, int retryAttemptCount, out TimeSpan sleepDuration)
    {
        if (exception is RpcException rpcException
            && OtlpRetry.TryGetGrpcRetryResult(rpcException.StatusCode, this.deadline, rpcException.Trailers, retryAttemptCount, out var retryResult))
        {
            sleepDuration = retryResult.RetryDelay;
            return true;
        }

        sleepDuration = default;
        return false;
    }
}

Example of retry via persistent storage

internal class TracePersistentStorageTransmissionHandler : OtlpExporterTransmissionHandler<ExportTraceServiceRequest>
{
    private PersistentBlobProvider persistentBlobProvider;
    private System.Timers.Timer timer;

    public TracePersistentStorageTransmissionHandler()
    {
        this.persistentBlobProvider = new FileBlobProvider(@"c:\temp\otlp\trace");
        this.timer = new System.Timers.Timer();
        this.timer.Elapsed += this.RetryRequestFromStorage;
        this.timer.Interval = 20000;
        this.timer.AutoReset = true;
        this.timer.Start();
    }

    protected override bool OnSubmitRequestExceptionThrown(ExportTraceServiceRequest request, Exception exception)
    {
        if (this.persistentBlobProvider.TryCreateBlob(request.ToByteArray(), out _))
        {
            return true;
        }

        return this.OnHandleDroppedRequest(request);
    }

    private void RetryRequestFromStorage(object sender, ElapsedEventArgs e)
    {
        int fileCount = 0;
        while (fileCount < 10)
        {
            if (this.persistentBlobProvider.TryGetBlob(out var blob))
            {
                if (blob != null && blob.TryLease(20000) && blob.TryRead(out var data))
                {
                    var request = new ExportTraceServiceRequest();
                    request.MergeFrom(data);
                    this.RetryRequest(request, out var exception);
                    if (exception == null)
                    {
                        blob.TryDelete();
                    }
                }
            }
            else
            {
                break;
            }

            fileCount++;
        }
    }
}

Exporters can enable retries based on the user selected retry mechanism. The selection will be via feature flag that will be introduced in follow up PRs.

 

Merge requirement checklist

  • CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (6250307) 83.38% compared to head (9729f43) 83.09%.
Report is 32 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4872      +/-   ##
==========================================
- Coverage   83.38%   83.09%   -0.30%     
==========================================
  Files         297      272      -25     
  Lines       12531    11983     -548     
==========================================
- Hits        10449     9957     -492     
+ Misses       2082     2026      -56     
Flag Coverage Δ
unittests ?
unittests-Solution-Experimental 82.99% <36.00%> (?)
unittests-Solution-Stable 83.06% <36.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...etryProtocol/Implementation/ExperimentalOptions.cs 100.00% <ø> (ø)
...tation/OpenTelemetryProtocolExporterEventSource.cs 100.00% <100.00%> (ø)
...rotocol/Implementation/OtlpLogRecordTransformer.cs 93.45% <100.00%> (ø)
...tation.AspNetCore/Implementation/HttpInListener.cs 89.72% <100.00%> (+0.14%) ⬆️
...AspNetCore/Implementation/HttpInMetricsListener.cs 89.47% <ø> (ø)
...plementation/HttpWebRequestActivitySource.netfx.cs 80.77% <100.00%> (ø)
...emetry/Metrics/Exemplar/SimpleExemplarReservoir.cs 80.43% <100.00%> (ø)
...mentation/Retry/OtlpExporterTransmissionHandler.cs 0.00% <0.00%> (ø)

... and 34 files with indirect coverage changes

@vishweshbankwar vishweshbankwar changed the title Add retry base for otlp Add base class for implementing retry strategies in otlp exporter Sep 25, 2023
@vishweshbankwar vishweshbankwar marked this pull request as ready for review September 25, 2023 22:12
@vishweshbankwar vishweshbankwar requested a review from a team September 25, 2023 22:12
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2023

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Oct 3, 2023
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Oct 11, 2023
@vishweshbankwar vishweshbankwar marked this pull request as draft January 23, 2024 17:28
@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jan 24, 2024
@vishweshbankwar vishweshbankwar marked this pull request as ready for review January 25, 2024 23:55
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

#nullable enable
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@CodeBlanch CodeBlanch added the pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package label Jan 26, 2024
Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

@CodeBlanch CodeBlanch merged commit eaf8316 into open-telemetry:main Jan 26, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants