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: Forward SetParentProvider to children of CompositeProcessor #3368

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Jun 15, 2022

Changes

Forward SetParentProvider calls to child processors when using CompositeProcessor.

TODOs

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Tests

@CodeBlanch CodeBlanch requested a review from a team June 15, 2022 20:11
@CodeBlanch
Copy link
Member Author

@alanwest You mentioned something on SIG about ParentProvider not working with custom processors. Think this could be why?

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM with a changelog (adding a unit test would be even better).

@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #3368 (e9e4379) into main (8b1521f) will increase coverage by 0.24%.
The diff coverage is 95.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3368      +/-   ##
==========================================
+ Coverage   85.80%   86.04%   +0.24%     
==========================================
  Files         268      268              
  Lines        9444     9460      +16     
==========================================
+ Hits         8103     8140      +37     
+ Misses       1341     1320      -21     
Impacted Files Coverage Δ
...rter.InMemory/InMemoryExporterLoggingExtensions.cs 90.90% <88.88%> (-9.10%) ⬇️
...penTelemetry.Exporter.InMemory/InMemoryExporter.cs 92.30% <100.00%> (ø)
...rter.InMemory/InMemoryExporterMetricsExtensions.cs 97.50% <100.00%> (ø)
src/OpenTelemetry/CompositeProcessor.cs 95.31% <100.00%> (+0.31%) ⬆️
.../OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs 94.44% <100.00%> (+0.21%) ⬆️
src/OpenTelemetry/Trace/TracerProviderSdk.cs 96.42% <100.00%> (+0.02%) ⬆️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 76.47% <0.00%> (+2.94%) ⬆️
...metryProtocol/Implementation/ActivityExtensions.cs 94.50% <0.00%> (+3.29%) ⬆️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 77.27% <0.00%> (+40.90%) ⬆️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 78.57% <0.00%> (+42.85%) ⬆️

@cijothomas
Copy link
Member

@alanwest You mentioned something on SIG about ParentProvider not working with custom processors. Think this could be why?

#3241 This!
@vishweshbankwar You have also reported the same earlier.

Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

Yes, this is a good fix.

Technically it resolves the initial issue that led to #3241, I think we can close it if we want. I still think the use case of filtering spans from getting exported may be better served by an example of a filtering exporter, but I can see the value of the filtering processor as well.

@CodeBlanch
Copy link
Member Author

So this fix actually won't help the MyFilteringProcessor referenced on #3241 😢 Because it is hiding the inner processor on a field.

We could make internal virtual void SetParentProvider(BaseProvider parentProvider) public to allow that processor to forward the set to its inner processor field. Or we could rewrite the sample like this, and then it would work:

internal class MyFilteringProcessor : CompositeProcessor<Activity>
{
    private readonly Func<Activity, bool> filter;

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

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

@alanwest
Copy link
Member

Oh good call, thanks you've jogged my memory of the problem here.

Or we could rewrite the sample like this

Yes! Personally, I like this idea idea over making SetParentProvider public. Pretty sure this whole concept of a parent provider is kind of a .NET specific need, so I guess I'm inclined to keep its public surface area small if possible.

@CodeBlanch
Copy link
Member Author

I like this idea idea over making SetParentProvider public.

I'm good with that I'll do as a follow-up.

@cijothomas cijothomas merged commit a58c7a3 into open-telemetry:main Jun 16, 2022
@CodeBlanch CodeBlanch deleted the compositeprocessor-setparentprovider-fix branch June 16, 2022 16:43
alanwest added a commit that referenced this pull request Jun 27, 2022
* Added Jaeger Propagator to Opentelemetry.Extensions.Propagators (#3309)

* Remove unnecessary bullet in CHANGELOG.md (#3352)

Co-authored-by: Cijo Thomas <[email protected]>

* Fix OTLP test (#3357)

* Show that test is not doing what you might think it does

* More asserts the merrier

* Show this little test that it has potential

* improve test coverage: InMemoryExporter & IDeferredMeterProviderBuilder (#3345)

* [SDK] Circular buffer tweaks + cpu pressure test (#3349)

* CircularBuffer tweaks and cpu pressure test.

* Switch to Volatile.Read.

* Perf tweaks.

* Remove race check in debug after doing more testing.

Co-authored-by: Cijo Thomas <[email protected]>

* Fix event name logic + support null categoryname. (#3359)

* In-memory Exporter: Buffer log scopes (#3360)

* Buffer log scopes when using in-memory exporter.

* CHANGELOG update.

* Code review.

* Tests.

* CHANGELOG tweak.

* SDK: Forward SetParentProvider to children of CompositeProcessor (#3368)

* Examples: Fix ParentProvider not being set on MyFilteringProcessor (#3370)

* Fix ParentProvider not being set on MyFilteringProcessor example.

* Added XML comments.

* Tweak.

* Typo.

* Logs: Add helper ctors & forceflush on OpenTelemetryLoggerProvider (#3364)

* Add helper ctors & forceflush on OpenTelemetryLoggerProvider.

* CHANGELOG update.

* Unit tests.

* Code review.

* Code review.

* Tweak.

* SDK: Nullable annotations for base classes & batch + shims to enable compiler features (#3374)

* Nullable annotations and shims for SDK base classes & batch.

* Target updates.

* Remove System.Collections.Immutable ref.

* ApiCompat attribute exclusions.

* ASPNETCore instrumentation to populate httpflavor tag (#3372)

* improve test coverage: InMemoryExporter SnapshotMetric (#3344)

* Fix AspNetCore metrics to use correct value for http.flavor (#3379)

* Fix AspNetCore metrics to use correct value for http.flavor

* word better

* Logs: Add LogRecordData (#3378)

* Add LogRecordData and hook up to LogRecord.

* CHANGELOG update.

* Code review.

* Remove SetHttpFlavor from Http instrumentations (#3381)

* Asp.Net Core trace instrumentation to populate http schema tag (#3392)

* Try asp.net core tests with inproc server (#3394)

* Dedupe IsPackable (#3398)

* Remove AspNet and AspNet.TelemetryHttpModule instrumentation projects (#3397)

* Handle possible exception when initializing the default service name (#3405)

* HttpClient: Invoke Enrich when SocketException = HostNotFound (#3407)

* Add & use ConfigureResource API. (#3307)

Co-authored-by: tyler jago <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Cijo Thomas <[email protected]>
Co-authored-by: Timothy Mothra <[email protected]>
Co-authored-by: Mikel Blanchard <[email protected]>
Co-authored-by: Reiley Yang <[email protected]>
Co-authored-by: Utkarsh Umesan Pillai <[email protected]>
Co-authored-by: Christian Neumüller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants