-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feat(traceview): Add previous trace link #85627
Open
Lms24
wants to merge
11
commits into
master
Choose a base branch
from
lms/feat-previous-trace-links
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+185
−0
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
fa00e81
[WIP] feat(traceview): Add previous trace link
Lms24 215e6aa
create trace link navigation button
s1gr1d ac8c012
check if trace of link is available
s1gr1d b721914
remove link skeleton
s1gr1d 4ecdfc0
Merge branch 'master' into lms/feat-previous-trace-links
s1gr1d 2bfc765
fix type error and use isEmptyTrace
s1gr1d cea338f
change rendering logic
s1gr1d 3c4638f
add timestamps
s1gr1d 1f53aff
Merge branch 'master' into lms/feat-previous-trace-links
s1gr1d b85643f
add feature flag
s1gr1d 18438bd
rename feature flag
s1gr1d File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
147 changes: 147 additions & 0 deletions
147
.../app/views/performance/newTraceDetails/traceLinksNavigation/traceLinkNavigationButton.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,147 @@ | ||
import {useMemo} from 'react'; | ||
import styled from '@emotion/styled'; | ||
|
||
import type { | ||
SpanLink, | ||
TraceContextType, | ||
} from 'sentry/components/events/interfaces/spans/types'; | ||
import Link from 'sentry/components/links/link'; | ||
import {normalizeDateTimeParams} from 'sentry/components/organizations/pageFilters/parse'; | ||
import {Tooltip} from 'sentry/components/tooltip'; | ||
import {IconChevron} from 'sentry/icons'; | ||
import {t} from 'sentry/locale'; | ||
import {space} from 'sentry/styles/space'; | ||
import {useLocation} from 'sentry/utils/useLocation'; | ||
import useOrganization from 'sentry/utils/useOrganization'; | ||
import {useTrace} from 'sentry/views/performance/newTraceDetails/traceApi/useTrace'; | ||
import {isEmptyTrace} from 'sentry/views/performance/newTraceDetails/traceApi/utils'; | ||
import {getTraceDetailsUrl} from 'sentry/views/performance/traceDetails/utils'; | ||
|
||
// Currently, we only support previous but component can be used for 'next trace' in the future | ||
type ConnectedTraceConnection = 'previous'; // | 'next'; | ||
|
||
const LINKED_TRACE_MAX_DURATION = 3600; // 1h in seconds | ||
|
||
function useIsTraceAvailable( | ||
traceLink?: SpanLink, | ||
previousTraceTimestamp?: number | ||
): { | ||
isAvailable: boolean; | ||
isLoading: boolean; | ||
} { | ||
const trace = useTrace({ | ||
traceSlug: traceLink?.trace_id, | ||
timestamp: previousTraceTimestamp, | ||
}); | ||
|
||
const isAvailable = useMemo(() => { | ||
if (!traceLink) { | ||
return false; | ||
} | ||
|
||
return Boolean(trace.data && !isEmptyTrace(trace.data)); | ||
}, [traceLink, trace]); | ||
|
||
return { | ||
isAvailable, | ||
isLoading: trace.isLoading, | ||
}; | ||
} | ||
|
||
type TraceLinkNavigationButtonProps = { | ||
currentTraceTimestamps: {end?: number; start?: number}; | ||
direction: ConnectedTraceConnection; | ||
isLoading?: boolean; | ||
traceContext?: TraceContextType; | ||
}; | ||
|
||
export function TraceLinkNavigationButton({ | ||
direction, | ||
traceContext, | ||
isLoading, | ||
currentTraceTimestamps, | ||
}: TraceLinkNavigationButtonProps) { | ||
const organization = useOrganization(); | ||
const location = useLocation(); | ||
|
||
const traceLink = traceContext?.links?.find( | ||
link => link.attributes?.['sentry.link.type'] === `${direction}_trace` | ||
); | ||
|
||
// We connect traces over a 1h period - As we don't have timestamps of the linked trace, it is calculated based on this timeframe | ||
const linkedTraceTimestamp = | ||
direction === 'previous' && currentTraceTimestamps.start | ||
? currentTraceTimestamps.start - LINKED_TRACE_MAX_DURATION // Earliest start times of previous trace | ||
: // : direction === 'next' && currentTraceTimestamps.end | ||
// ? currentTraceTimestamps.end + LINKED_TRACE_MAX_DURATION | ||
undefined; | ||
|
||
const dateSelection = useMemo( | ||
() => normalizeDateTimeParams(location.query), | ||
[location.query] | ||
); | ||
|
||
const {isAvailable: isLinkedTraceAvailable} = useIsTraceAvailable( | ||
traceLink, | ||
linkedTraceTimestamp | ||
); | ||
|
||
if (isLoading) { | ||
// We don't show a placeholder/skeleton here as it would cause layout shifts most of the time. | ||
// Most traces don't have a next/previous trace and the hard to avoid layout shift should only occur if the actual button can be shown. | ||
return null; | ||
} | ||
|
||
if (traceLink && isLinkedTraceAvailable) { | ||
return ( | ||
<TraceLink | ||
color="gray500" | ||
to={getTraceDetailsUrl({ | ||
traceSlug: traceLink.trace_id, | ||
spanId: traceLink.span_id, | ||
dateSelection, | ||
timestamp: linkedTraceTimestamp, | ||
location, | ||
organization, | ||
})} | ||
> | ||
<IconChevron direction="left" /> | ||
<TraceLinkText>{t('Go to Previous Trace')}</TraceLinkText> | ||
</TraceLink> | ||
); | ||
} | ||
|
||
if (traceLink?.sampled === false) { | ||
return ( | ||
<StyledTooltip | ||
position="right" | ||
title={t( | ||
'Trace contains a link to unsampled trace. Increase traces sample rate in SDK settings to see more connected traces' | ||
)} | ||
> | ||
<TraceLinkText>{t('Previous trace not available')}</TraceLinkText> | ||
</StyledTooltip> | ||
); | ||
} | ||
|
||
// If there is no linked trace or an undefined sampling decision | ||
return null; | ||
} | ||
|
||
const StyledTooltip = styled(Tooltip)` | ||
padding: ${space(0.5)} ${space(1)}; | ||
text-decoration: underline dotted | ||
${p => (p.disabled ? p.theme.gray300 : p.theme.gray300)}; | ||
`; | ||
|
||
const TraceLink = styled(Link)` | ||
font-weight: ${p => p.theme.fontWeightNormal}; | ||
color: ${p => p.theme.subText}; | ||
padding: ${space(0.25)} ${space(0.5)}; | ||
display: flex; | ||
align-items: center; | ||
`; | ||
|
||
const TraceLinkText = styled('span')` | ||
line-height: normal; | ||
`; |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 trace fetching performance looks okay on our current system, but the EAP equivalent is currently too expensive for this purpose IMO. It's still in development (only internal users) so not currently a blocker and hopefully this is resolved before release, but if it doesn't then we'll probably want to replace this with an endpoint specifically for this check (to avoid loading a full trace) as you mentioned on Slack.
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.
Today making use of an explore query as such should work right @mjq? It should be future proof
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.
Ah, great call. We could make an explore query for
count(span.duration)
wheretrace:<trace_id>
, and if the result is > 0, show the trace link?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.
I can't see a way to do this in the explore interface, but if we could instead search for
min(timestamp)
we'd be able to both confirm trace existence and also get the timestamp needed for the trace details URL in a single call, which would be slick.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.
@mjq we don't use the events endpoint for that query we use a
/traces/
endpoint that comes withnumOfspans
and thetimestamp
linked to the trace.The only concern I have now is that, since the tracelink doesn't come with a timestamp, how do we set the date range for that explore query? Either of, 'querying by the current trace's timestamp with a buffer' or
'quering by max date range at all times' doesn't seem to be right 🤔
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.
At this point I think I'd suggest putting this behind a feature flag but otherwise merging it as is so we can start gathering real data from a subset of customers. We can loop back on optimizing the call with one of the options in this thread (or something else) after that. None of the options are perfect so I imagine it'll take some iterating, but with the feature flag we don't have to block the work so far on that iteration. How do you feel @s1gr1d @Lms24 ? (I guess this addresses "does this need a feature flag guard?" from the PR description 😄 )
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.
Based on this thread, I am not 100% sure what should be the path forward now :D
Should we either:
And if we use another request here: Which one do you mean exactly here? I am not too familiar with the codebase so maybe you could tell me which hooks are already available for that.
As for the timestamp: The attached "previous trace" data only provides the
spanId
andtraceId
so I cannot know a timestamp at the point of fetching for this trace but we can define a time range. We know that the end timestamp of the previous trace would be anytime before the start timestamp of the trace that is currently shown in the trace view. And if we say that the maximum duration between linked traces is 1h, we can define the start time as "end time minus 1h". So then we would have this for the previous trace query: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.
shouldn't this be start time of current trace minus 1h?