-
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
[es] Refactor the ES SpanWriter to make it reusable for v2 APIs #6796
Conversation
Signed-off-by: Manik2708 <[email protected]>
@mahadzaryab1 @yurishkuro I am bit confused for the directory structure of model. I have kept it in |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6796 +/- ##
=======================================
Coverage 96.03% 96.04%
=======================================
Files 364 364
Lines 20702 20712 +10
=======================================
+ Hits 19882 19892 +10
Misses 626 626
Partials 194 194
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This reverts commit 6f826da. Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
@yurishkuro I have refactored with the way you said. Please review, once the apporach here is approved, same will be repeated for reader. But one thing which I want to ask is should we include |
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
This reverts commit 68b066f. Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
@yurishkuro Does the new changes make sense? |
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
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.
lgtm
…ertracing#6796) ## Which problem is this PR solving? Fixes a part of jaegertracing#6458 ## Description of the changes - The JsonSpanWriter accepts the json span instead of jaeger span which makes it reusabe in v2 storage APIs ## How was this change tested? - Unit Tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: Manik2708 <[email protected]>
Which problem is this PR solving?
Fixes a part of #6458
Description of the changes
How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:npm run lint
andnpm run test