-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 Elasticsearch storage support for adaptive sampling #5158
Add Elasticsearch storage support for adaptive sampling #5158
Conversation
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
Raised PR to get to know from reviewers whether I am on the right track or not. |
adding tests |
Signed-off-by: Pushkar Mishra <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5158 +/- ##
==========================================
+ Coverage 94.49% 94.56% +0.07%
==========================================
Files 334 336 +2
Lines 19303 19501 +198
==========================================
+ Hits 18241 18442 +201
+ Misses 875 871 -4
- Partials 187 188 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- run
make fmt
- please add tests to all new functionality
- enable integration tests (see plugin/storage/integration/integration.go:423)
Signed-off-by: Pushkar Mishra <[email protected]>
Enabling integration tests |
Signed-off-by: Pushkar Mishra <[email protected]>
The Tests are failing because the
|
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
any idea @yurishkuro, why it is failing on es v5 but not on v7 and v8. |
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
added rollover support |
Please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost there!
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
tests are also failing. looks like i have to add adaptive sampling in this |
heh, I left a comment about that, but why would have any impact on the tests? The cleaner is just over-eager in which indices it removes, but unless the integration tests are actually testing for this behavior I would expect to be a no-op (because either rollover will create extra indices and things would work, or it wouldn't create them and cleaner will be no-op). are you able to run these integration tests locally? |
I see this error, maybe you're not passing extra flags inside the integration test to actually generate sampling-related index?
|
Signed-off-by: Pushkar Mishra <[email protected]>
YES, but sometimes it fails with a timeout, in that case, I rely on gh workflow
working on it |
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
🤞everything seems good now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test