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 policy sampling processor - 1/2 #1786

Conversation

jpkrohling
Copy link
Member

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

Description: This PR creates a new sampling processor based on the tail-based sampling, without the logic that is needed for the grouping of traces, which can be accomplished with the groupbytrace processor. Once this processor is merged, we can deprecate the tail-based sampler in favor of this + groupbytrace.

Testing: unit tests + manual tests (final solution).

Documentation: readme file.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@jpkrohling jpkrohling requested a review from a team December 8, 2020 10:10
@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #1786 (4579f6f) into master (d48fa8d) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1786      +/-   ##
==========================================
+ Coverage   89.85%   89.87%   +0.02%     
==========================================
  Files         376      378       +2     
  Lines       18180    18216      +36     
==========================================
+ Hits        16336    16372      +36     
  Misses       1383     1383              
  Partials      461      461              
Flag Coverage Δ
integration 69.77% <ø> (ø)
unit 88.57% <100.00%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
processor/policysamplingprocessor/factory.go 100.00% <100.00%> (ø)
processor/policysamplingprocessor/metrics.go 100.00% <100.00%> (ø)
processor/groupbytraceprocessor/event.go 95.96% <0.00%> (-0.81%) ⬇️
receiver/carbonreceiver/transport/tcp_server.go 67.00% <0.00%> (+1.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d48fa8d...4579f6f. Read the comment docs.

)

// PolicyType indicates the type of sampling policy.
type PolicyType string
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to restrict this to an enumerated list? If someone wants to create their own proprietary policy they will need to fork this repo and make changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review! This part is inherited from the current tail-based sampling, but you are right. In fact, I think we could discuss making this go in another direction: instead of one "policysamplingprocessor", we could be having two or three processors: ratelimitingprocessor (rate_limiting from this PR) and attributesamplingprocessor (string and int policies from this PR). The always sampling would just disappear, as it's not really useful IMO.

What do you think?

@pmm-sumo
Copy link
Contributor

pmm-sumo commented Dec 9, 2020

As we discussed during the SIG, here's another attempt at making tail-sampling processor more robust: https://github.com/SumoLogic/opentelemetry-collector-contrib/tree/master/processor/cascadingfilterprocessor

@jpkrohling
Copy link
Member Author

Closing this for now, as this will probably become two other processors. See #1797.

@jpkrohling jpkrohling closed this Dec 10, 2020
dyladan referenced this pull request in dynatrace-oss-contrib/opentelemetry-collector-contrib Jan 29, 2021
Make the comment and log message match the actual constant used in the deprecation logic.
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
… name (#1777) (#1786)

Signed-off-by: lastchiliarch <lastchiliarch@163.com>

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
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