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

[docs] Adjust filtering processor guidance #3834

Merged
merged 2 commits into from
Oct 27, 2022

Conversation

CodeBlanch
Copy link
Member

The previous "FilteringProcessor" example required manually constructing the exporter and passing it into the processor. Many exporters do not have a public ctor so it is not possible to construct them.

Changes

Switched the example to toggle Activity.Recorded instead. This works because BatchActivityExportProcessor and SimpleActivityExportProcessor both no-op for this case.

@CodeBlanch CodeBlanch requested a review from a team October 27, 2022 22:40
@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Merging #3834 (b90b4fa) into main (d954c5b) will decrease coverage by 0.08%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3834      +/-   ##
==========================================
- Coverage   87.48%   87.40%   -0.09%     
==========================================
  Files         280      280              
  Lines       10767    10767              
==========================================
- Hits         9420     9411       -9     
- Misses       1347     1356       +9     
Impacted Files Coverage Δ
...Propagators/OpenTelemetryPropagatorsEventSource.cs 87.50% <0.00%> (-12.50%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 76.47% <0.00%> (-5.89%) ⬇️
...lementation/SqlClientInstrumentationEventSource.cs 70.83% <0.00%> (-4.17%) ⬇️
...lemetry/Internal/SelfDiagnosticsConfigRefresher.cs 86.53% <0.00%> (-2.89%) ⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 80.15% <0.00%> (-2.39%) ⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs 100.00% <0.00%> (+2.85%) ⬆️

@vishweshbankwar
Copy link
Member

@CodeBlanch - Filter by setting activity.IsAllDataRequested = false during OnStart. Should we document that as well?. (That will prevent all exporters to export it).

@CodeBlanch
Copy link
Member Author

@vishweshbankwar Customer wants to filter based on some data on the Activity. Duration for example. OnEnd is really the most general-purpose spot we have to filter where all the data will be available. We could also have an OnStart example which uses IsAllDataRequested but more to document there to explain the limited dataset. My concern would be having both might be confusing. @cijothomas How you feel about it?

@cijothomas
Copy link
Member

We should be only showing the OnEnd based filter as shown in this PR. There is not much value in showing Filter based on OnStart(), as Sampler is a better fit there. OnEnd is more like "tail" based filtering, based on data not available at SpanStart/Sample time.

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM

@cijothomas cijothomas merged commit f921ae3 into open-telemetry:main Oct 27, 2022
@CodeBlanch CodeBlanch deleted the filter-processor branch October 28, 2022 17:49
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.

3 participants