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

Jump to bottom

Add support for composite sampling policy to the tailsampler #4958

Merged
merged 8 commits into from
Oct 21, 2021
Merged

Add support for composite sampling policy to the tailsampler #4958

merged 8 commits into from
Oct 21, 2021

Conversation

vikrambe
Copy link
Contributor

Is your feature request related to a problem? Please describe.
#1410
Design Doc - Added support for composite policy in tailsampling processor. This would help in grouping sampling policies and rate limiting them. https://docs.google.com/document/d/10wpIv3TtXgOik05smHm3nYeBX48Bj76TCMxPy8e1NZw/edit#heading=h.ecy5l2puwtp4
This is a split. Refer PR open-telemetry/opentelemetry-collector#1894 (comment)

Due to EasyCLA issue opening a new PR: #4396

Link to tracking Issue: 1306

@vikrambe vikrambe requested a review from jpkrohling as a code owner August 29, 2021 10:36
@vikrambe vikrambe requested a review from a team August 29, 2021 10:36
@vikrambe vikrambe mentioned this pull request Aug 29, 2021
Closed
@vikrambe
Copy link
Contributor Author

@jpkrohling Requesting your review on this PR #4958.

vikrambe
vikrambe commented Aug 29, 2021
View reviewed changes
Copy link
Contributor Author

@vikrambe vikrambe left a comment

Choose a reason for hiding this comment

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

@jpkrohling FYI... I have pulled in open comments from old PR

processor/tailsamplingprocessor/composite_helper.go Show resolved Hide resolved
processor/tailsamplingprocessor/config.go Show resolved Hide resolved
processor/tailsamplingprocessor/internal/sampling/composite.go
// it is not certain and it is also additional performance penalty when we are
// already under a memory (and possibly CPU) pressure situation.
//
// For now we are playing safe and go with #3. Investigating alternate options
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpkrohling Playing safe here probably means dropping everything, as sampling everything might put even more pressure on memory.
@vikrambe Agreed. We need to think through this. Any suggestion ?

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it work to just return NotSampled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, it should work. Let me run some tests and take this change in next PR.

processor/tailsamplingprocessor/internal/sampling/composite_test.go Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2021

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Sep 6, 2021
@jpkrohling jpkrohling removed the Stale label Sep 6, 2021
@vikrambe
Copy link
Contributor Author

vikrambe commented Sep 8, 2021

@jpkrohling Appreciate your review on this

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Sep 15, 2021
@jpkrohling jpkrohling removed the Stale label Sep 15, 2021
jpkrohling
jpkrohling approved these changes Sep 15, 2021
View reviewed changes
processor/tailsamplingprocessor/internal/sampling/composite.go
// restarts at the beginning of each second.
// Current counters and rate limits are kept separately for each subpolicy.

currSecond := c.timeProvider.getCurSecond()
Copy link
Member

Choose a reason for hiding this comment

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

I think I have asked this before, and sorry if you answered it as well, but did you try to use Go's rate limiting from the stdlib? https://pkg.go.dev/golang.org/x/time/rate

If you can't use that for some reason, perhaps it's worth a comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpkrohling Thanks, I like this idea. I had commented that i will take it in next PR. I need to run some tests to confirm the behaviour once.

processor/tailsamplingprocessor/internal/sampling/composite.go
// it is not certain and it is also additional performance penalty when we are
// already under a memory (and possibly CPU) pressure situation.
//
// For now we are playing safe and go with #3. Investigating alternate options
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it work to just return NotSampled?

@jpkrohling
Copy link
Member

This LGTM, and the remaining comments can be handled later. Just make sure the lint checks are passing.

@vikrambe
Copy link
Contributor Author

@jpkrohling please approve workflow

@vikrambe
Copy link
Contributor Author

@jpkrohling I am not authorised to merge this PR. Please merge it

@vikrambe
Copy link
Contributor Author

@jpkrohling @dmitryax Can one of you please merge this PR ?

@vikrambe
Copy link
Contributor Author

@jpkrohling @dmitryax @bogdandrutu Can we merge this PR ?

@vikrambe
Copy link
Contributor Author

vikrambe commented Sep 28, 2021 •
edited
Loading

@jpkrohling Need your help in getting this merged

@vikrambe
Copy link
Contributor Author

vikrambe commented Oct 2, 2021 •
edited
Loading

@jpkrohling @dmitryax @bogdandrutu Can one of you merge this PR ?

@jpkrohling
Copy link
Member

Sorry, I was out on PTO and couldn't follow up: @bogdandrutu, @tigrannajaryan, please merge this one once @vikrambe fixes the conflict in the readme file.

tigrannajaryan
tigrannajaryan approved these changes Oct 7, 2021
View reviewed changes
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Needs a conflict resolution before I can merge.

@vikrambe
Copy link
Contributor Author

vikrambe commented Oct 9, 2021

Needs a conflict resolution before I can merge.

@tigrannajaryan I have fixed the README conflict. Please merge it.

@jpkrohling
Copy link
Member

@vikrambe I appreciate your patience and perseverance in getting this done. I just approved the tests to be executed.

@tigrannajaryan
Copy link
Member

@vikrambe the build fails.

composite_helper_test.go:28:63: NewID not declared by package config (typecheck)
		ProcessorSettings:       config.NewProcessorSettings(config.NewID(typeStr)),
		                                                            ^
make[2]: *** [../../Makefile.Common:95: lint] Error 1

@vikrambe
Copy link
Contributor Author

@vikrambe the build fails.

composite_helper_test.go:28:63: NewID not declared by package config (typecheck)
		ProcessorSettings:       config.NewProcessorSettings(config.NewID(typeStr)),
		                                                            ^
make[2]: *** [../../Makefile.Common:95: lint] Error 1

@jpkrohling @tigrannajaryan Thanks for reviewing this PR. Sorry for the delay i was caught up with other work. I have fixed the test.

@tigrannajaryan tigrannajaryan changed the title 1306:Adding support for composite sampling policy to the tailsampler Add support for composite sampling policy to the tailsampler Oct 21, 2021
@tigrannajaryan tigrannajaryan merged commit b90d1f0 into open-telemetry:main Oct 21, 2021
@tigrannajaryan
Copy link
Member

Thank you @vikrambe

@jpkrohling
Copy link
Member

@vikrambe, again, thank you for not giving up!

@vikrambe
Copy link
Contributor Author

@jpkrohling @tigrannajaryan Thanks for all the help and support.

@vikrambe vikrambe mentioned this pull request Oct 22, 2021
Closed
@vikrambe vikrambe mentioned this pull request Dec 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tigrannajaryan tigrannajaryan tigrannajaryan approved these changes

@jpkrohling jpkrohling jpkrohling approved these changes

Assignees

@dmitryax dmitryax

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vikrambe @jpkrohling @tigrannajaryan @dmitryax