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.

Sign up for GitHub

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

Examples: Fix ParentProvider not being set on MyFilteringProcessor #3370

Merged
merged 5 commits into from
Jun 16, 2022
Merged

Examples: Fix ParentProvider not being set on MyFilteringProcessor #3370

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f965fa5
Fix ParentProvider not being set on MyFilteringProcessor example.
CodeBlanch Jun 15, 2022
8473a05
Merge branch 'main' into example-filtering-processor-fix
CodeBlanch Jun 16, 2022
bd4ce45
Added XML comments.
CodeBlanch Jun 16, 2022
7113fa0
Tweak.
CodeBlanch Jun 16, 2022
5954cfa
Typo.
CodeBlanch Jun 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions docs/trace/extending-the-sdk/MyFilteringProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,14 @@
using System.Diagnostics;
using OpenTelemetry;

internal class MyFilteringProcessor : BaseProcessor<Activity>
internal class MyFilteringProcessor : CompositeProcessor<Activity>
Copy link
Member

Choose a reason for hiding this comment

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

Purely from the reader's perspective - I guess folks would question "why would I need to derive from CompositeProcessor?".

Copy link
Member

@alanwest alanwest Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

🤔 this is a good question, and I can't think of simple explanation to give users. The alternative to making SetParentProvider public is problematic for similar reasons. I'd struggle to come up with a concise way to explain to users under what circumstances they need to be sure to call SetParentProvider.

In either case the explanation is basically: if you're newing up a processor and you're not directly passing it to providerBuilder.AddProcessor() (i.e., it's nested in another processor or something) then you gotta make sure that the parent provider gets set - which in turn ensures the processor has access to the configured resource... which may or may not be important depending on the processor

which then raises questions of why the parent provider needs to be set, why does it need access to a resource, and when is all of this important

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a good solution for this! Agree with @alanwest exposing SetParentProvider feels wrong. I just added some XML comments explaining (or hinting at) what is going on in the example. That might help?

We could introduce a class to make it more self-explanatory like...

public class DelegatingProcessor<T> : CompositeProcessor<T>
{
   public DelegatingProcessor(BaseProcessor<T> innerProcessor)
      : base(new[] { innerProcessor ?? throw new ArgumentNullException(nameof(innerProcessor) })
   {
   }
}

...or...

    public class DelegatingProcessor<T> : BaseProcessor<T>
    {
        public DelegatingProcessor(BaseProcessor<T> innerProcessor)
        {
            Guard.ThrowIfNull(innerProcessor);

            this.InnerProcessor = innerProcessor;
        }

        protected BaseProcessor<T> InnerProcessor { get; }

        public override void OnStart(T data)
        {
            this.InnerProcessor.OnStart(data);
        }

        public override void OnEnd(T data)
        {
            this.InnerProcessor.OnEnd(data);
        }

        internal override void SetParentProvider(BaseProvider parentProvider)
        {
            this.InnerProcessor.SetParentProvider(parentProvider);
        }

        protected override bool OnForceFlush(int timeoutMilliseconds)
        {
            return this.InnerProcessor.ForceFlush(timeoutMilliseconds);
        }

        protected override bool OnShutdown(int timeoutMilliseconds)
        {
            return this.InnerProcessor.Shutdown(timeoutMilliseconds);
        }

        protected override void Dispose(bool disposing)
        {
            if (disposing)
            {
                this.InnerProcessor.Dispose();
            }

            base.Dispose(disposing);
        }
    }

I doubt anyone will like that either, but it's an option 🤣

Copy link
Member

@reyang reyang Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Giving this is an example, I guess I can just close my eyes on it and click the approve button. 🤣

I guess there might be a solution to retrieve the "parent" processor's ParentProvider - based on callstack or context (should be reasonably performant since we only have to fetch it once when the ParentProvider is not yet set), but maybe it's just an overkill 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

I doubt anyone will like that either, but it's an option 🤣

Yea not yet excited about this idea. I do agree the naming offers a slight improvement, but I think it's worth sitting on a bit. Definitely think it's worth continuing this discussion in hopes of finding a better solution.

Copy link
Member

Choose a reason for hiding this comment

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

Something about this problem kinda feels similar to middleware branching. Like the filter processor effectively introduces a branch in the pipeline. Something like

if (filterFunc(span)) {
    // invoke the next process in the chain
} else {
    // terminate the pipeline - maybe call a noop processor or something
}

{
private readonly Func<Activity, bool> filter;
private readonly BaseProcessor<Activity> processor;

public MyFilteringProcessor(BaseProcessor<Activity> processor, Func<Activity, bool> filter)
: base(new[] { processor })
{
this.filter = filter ?? throw new ArgumentNullException(nameof(filter));
this.processor = processor ?? throw new ArgumentNullException(nameof(processor));
}

public override void OnEnd(Activity activity)
Expand All @@ -35,7 +34,7 @@ public override void OnEnd(Activity activity)
// only if the Filter returns true.
if (this.filter(activity))
{
this.processor.OnEnd(activity);
base.OnEnd(activity);
}
}
}