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

feat: Add sampling store support to Badger #4834

Merged
merged 7 commits into from
Oct 26, 2023
Merged

feat: Add sampling store support to Badger #4834

merged 7 commits into from
Oct 26, 2023

Conversation

slayer321
Copy link
Contributor

@slayer321 slayer321 commented Oct 11, 2023 •
edited
Loading

Which problem is this PR solving?

Related #3305

Description of the changes

  • Implemented badger db for sampling store

How was this change tested?

  • Added Unit test and also tested it with the already Implemented integration test

Checklist

@slayer321
Copy link
Contributor Author

slayer321 commented Oct 11, 2023 •
edited
Loading

Hey @yurishkuro , for now I have Implemented InsertThroughput and GetThroughput , and working on Implementing the other two methods. Will push those changes in some time.
Let me know your thoughts on current changes that I have I done.

Edit: Implemented InsertProbabilitiesAndQPS and GetLatestProbabilities as well.

@slayer321 slayer321 force-pushed the badger-sampling-store-implementation branch from c818f63 to 2f24d91 Compare October 11, 2023 14:50
@codecov
Copy link

codecov bot commented Oct 11, 2023 •
edited
Loading

Codecov Report

Attention: 46 lines in your changes are missing coverage. Please review.

Files Coverage Δ
plugin/storage/integration/integration.go 73.93% <100.00%> (+0.07%) ⬆️
plugin/storage/badger/factory.go 94.48% <0.00%> (-1.33%) ⬇️
plugin/storage/badger/samplingstore/storage.go 74.26% <74.26%> (ø)

📢 Thoughts on this report? Let us know!.

@slayer321 slayer321 force-pushed the badger-sampling-store-implementation branch from 5c75b90 to f3bd46d Compare October 13, 2023 13:24
@slayer321 slayer321 marked this pull request as ready for review October 13, 2023 13:27
@slayer321 slayer321 requested a review from a team as a code owner October 13, 2023 13:27
yurishkuro
yurishkuro reviewed Oct 16, 2023
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

minor comments, lgtm otherwise. If possible, try to add tests for conditions that can be tested.

@slayer321 slayer321 force-pushed the badger-sampling-store-implementation branch from e6d395c to 04f67c4 Compare October 16, 2023 13:39
@slayer321
feat: Implement badger db for sampling store
b3a413e 
Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
@slayer321
fix lint issue
6c72f42 
Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
@slayer321
implement InsertProbabilitiesAndQPS and GetLatestProbabilities
8900b39 
Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
@slayer321
handle error
8180f18 
Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
@slayer321
add suggested changes
6ee5863 
Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
@slayer321
fix failing CI for intergration test
32e24d6 
Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
@slayer321
minor suggested changes
2928d5e 
Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
@slayer321 slayer321 force-pushed the badger-sampling-store-implementation branch from ba1e613 to 2928d5e Compare October 17, 2023 13:15
@slayer321
Copy link
Contributor Author

Hey @yurishkuro , I have made the changes can you please take a look.

@yurishkuro yurishkuro added the changelog:new-feature Change that should be called out as new feature in CHANGELOG label Oct 26, 2023
@yurishkuro yurishkuro changed the title feat: Implement badger db for sampling store feat: Add sampling store support to Badger Oct 26, 2023
@yurishkuro yurishkuro merged commit f99eae5 into jaegertracing:main Oct 26, 2023
31 of 33 checks passed
@yurishkuro
Copy link
Member

Thanks!

This was referenced Nov 25, 2023
Closed
Merged
yurishkuro added a commit that referenced this pull request Nov 26, 2023
@yurishkuro
Declare all interfaces each factory implements (#4967)
4096fe9 
## Which problem is this PR solving?
- Part of #4963
- Follow up to #4834

## Description of the changes
- Explicitly declare all interfaces each factory implements
- Move these declarations to the main file for visibility

## How was this change tested?
- CI

Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro mentioned this pull request Mar 3, 2024
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro left review comments

Assignees
No one assigned
Labels
changelog:new-feature Change that should be called out as new feature in CHANGELOG
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@slayer321 @yurishkuro