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

[sdk] Add DelegatingProcessor #5282

Closed

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Jan 30, 2024

Relates to #5255

Changes

  • Adds a public DelegatingProcessor<T> class to SDK.

Details & reasoning

We don't stop users from making more derived versions of our built-in processors or wrapping them in other processors. An example of this is #5250.

Users who wrap one of the built-in processors may run into two things:

Because we can't prevent this, what we should do (IMHO) is give a way to do it safely: DelegatingProcessor

Here is how a user could write the FilterProcessor (from #5250) to be safe using DelegatingProcessor:

    internal sealed class FilteringProcessor<T> : DelegatingProcessor<T>
    {
        private readonly Func<T, bool> filter;

        public FilteringProcessor(BaseProcessor<T> inner, Func<T, bool> filter)
           : base(inner)
        {
            this.filter = filter;
        }

        public override void OnEnd(T data)
        {
            // Call the underlying processor
            // only if the Filter returns true.
            if (this.filter(data))
            {
                base.OnEnd(data);
            }
        }
    }

Public API changes

OpenTelemetry.dll:

namespace OpenTelemetry;

+public abstract class DelegatingProcessor<T> : BaseProcessor<T>
+{
+   protected DelegatingProcessor(BaseProcessor<T> innerProcessor) {}
+   public override void OnStart(T data) {}
+   public override void OnEnd(T data) {}
+   protected override bool OnForceFlush(int timeoutMilliseconds) {}
+   protected override bool OnShutdown(int timeoutMilliseconds) {}
+   protected override void Dispose(bool disposing) {}
+}

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, 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)

@CodeBlanch CodeBlanch added the pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package label Jan 30, 2024
@TimothyMothra
Copy link
Contributor

My first thought, are there any one-off Processors in the Test projects that this could replace?

Copy link

codecov bot commented Jan 30, 2024

Codecov Report

Attention: Patch coverage is 5.00000% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 82.91%. Comparing base (6250307) to head (3115a71).
Report is 98 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5282      +/-   ##
==========================================
- Coverage   83.38%   82.91%   -0.48%     
==========================================
  Files         297      273      -24     
  Lines       12531    11993     -538     
==========================================
- Hits        10449     9944     -505     
+ Misses       2082     2049      -33     
Flag Coverage Δ
unittests ?
unittests-Solution-Experimental 82.87% <5.00%> (?)
unittests-Solution-Stable 82.58% <5.00%> (?)

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

Files Coverage Δ
src/OpenTelemetry/Logs/LoggerProviderSdk.cs 92.00% <50.00%> (-0.86%) ⬇️
src/OpenTelemetry/DelegatingProcessor.cs 0.00% <0.00%> (ø)

... and 37 files with indirect coverage changes

Copy link
Contributor

github-actions bot commented Feb 7, 2024

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 Stale Issues and pull requests which have been flagged for closing due to inactivity and removed Stale Issues and pull requests which have been flagged for closing due to inactivity labels Feb 7, 2024
Copy link
Contributor

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 Stale Issues and pull requests which have been flagged for closing due to inactivity and removed Stale Issues and pull requests which have been flagged for closing due to inactivity labels Feb 15, 2024
Copy link
Contributor

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 Feb 24, 2024
Copy link
Contributor

github-actions bot commented Mar 3, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants