-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[v2] Implement GetDependencies for memory backend
#7154
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
Conversation
Signed-off-by: Manik2708 <[email protected]>
|
I have a question: Do we need to consider the case when start time is 0 but end time is not? Currently it will throw an error in this case! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7154 +/- ##
==========================================
- Coverage 96.21% 96.19% -0.03%
==========================================
Files 361 361
Lines 21779 21838 +59
==========================================
+ Hits 20955 21006 +51
- Misses 616 622 +6
- Partials 208 210 +2
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:
|
internal/storage/v2/memory/memory.go
Outdated
| SpanKind: span.Kind().String(), | ||
| } | ||
| m.storeOperation(serviceName, operation) | ||
| if startTime.IsZero() || endTime.IsZero() { |
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.
Why does this need to be in this function instead of in the tenant? The tenant has additional checks for time anyway, I don't see the value in splitting the logic
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.
The reason is: Tenant is just storing the traces. The logic in tenant expects that the start and end time of trace is already found because tenant doesn't iterates the spans it just sees if a trace with same trace id already exists and if yes then it appends the resource spans to that trace and if not then it creates a new trace. This same is hapenning here. Firstly we iterate the spans of a trace and find the start and end time and then send them to tenant. Now tenant sees if there exists a trace already, if yes then append the spans to that trace and update the start and end time (if bounds are out of the range of start and end time) and if not then just create new trace with start and time. How are we expected to find start and end time of trace in tenant?
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.
tenant can run the same loop you're running here to identify the timestamps. The memory storage itself has no use for those timestamps, so why does it need to know about them and increase the API surface of the tenant?
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.
Does the new way looks fine?
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Which problem is this PR solving?
Description of the changes
GetDependenciesfor memory backendHow was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test