feat: adding start time to spans and trace queries#435
Conversation
| traceProperties: [], | ||
| spanProperties: this.spanSpecifications | ||
| spanProperties: this.spanSpecifications, | ||
| spansTimeRange: this.getTimeRangeOrThrow() |
There was a problem hiding this comment.
Previously span time range was same as Selected time range. Keeping it the same now. Although, it is definitely incorrect. We should be able to fetch all spans here for the given trace.
There was a problem hiding this comment.
Ah, commented on this above - we should be able to fix this.
|
We have trace detail, trace waterfall, api trace detail, api trace waterfall, extended api trace and extended api trace waterfall. Phew! |
Codecov Report
@@ Coverage Diff @@
## main #435 +/- ##
==========================================
+ Coverage 85.68% 85.70% +0.02%
==========================================
Files 750 751 +1
Lines 15339 15393 +54
Branches 1816 1826 +10
==========================================
+ Hits 13143 13193 +50
- Misses 2165 2168 +3
- Partials 31 32 +1
Continue to review full report at Codecov.
|
| traceProperties: [], | ||
| spanProperties: this.spanSpecifications | ||
| spanProperties: this.spanSpecifications, | ||
| spansTimeRange: this.getTimeRangeOrThrow() |
There was a problem hiding this comment.
Ah, commented on this above - we should be able to fix this.
| export class TracingNavigationService { | ||
| public constructor(private readonly navigationService: NavigationService) {} | ||
|
|
||
| public navigateToTraceDetail(traceId: string, spanId?: string, startTime?: unknown): Observable<boolean> { |
There was a problem hiding this comment.
same comment about unknowns
|
updated |
This comment has been minimized.
This comment has been minimized.
…il.service.ts Co-authored-by: Aaron Steinfeld <45047841+aaron-steinfeld@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| }; | ||
| } | ||
|
|
||
| protected getTimeRangeOrThrow(): GraphQlTimeRange { |
There was a problem hiding this comment.
Should we do this SpanGraphQlQueryHandlerService as well? We are not using it at many places.
There was a problem hiding this comment.
I think it's worth doing, but can happen in a separate PR
There was a problem hiding this comment.
will put up a separate PR
| @ModelProperty({ | ||
| key: 'start-time', | ||
| required: false, | ||
| type: UNKNOWN_PROPERTY.type |
There was a problem hiding this comment.
Because i think we can't put union types in model properties. This field can be string, number or undefined
There was a problem hiding this comment.
But why not just say number (or string)? Given that we can't be as broad as we want (the code would work with string | number), it seems better to be narrower and force the value provider to match (vs having an overly broad type where some values would result in an error)
There was a problem hiding this comment.
Surely we could do that. The unknown pattern was there (may be i used it initially for a valid reason). With current design, i agree we could just force it to be of a particular type.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 participants
Description
Please include a summary of the change, motivation and context.
All spans and Traces queries may optionally use a startTime for building query time range.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.