Skip to content

feat(trace-view): Add UI fallback to old view for STs #73684

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 15, 2024
Merged

feat(trace-view): Add UI fallback to old view for STs #73684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
05bb2af
feat(trace-view): Add UI fallback to old view for STs
k-fish Jul 2, 2024
73ff278
Set deprecated properly
k-fish Jul 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 8 additions & 18 deletions static/app/components/quickTrace/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {backend, frontend, mobile, serverless} from 'sentry/data/platformCategor
import {IconFire} from 'sentry/icons';
import {t, tct, tn} from 'sentry/locale';
import type {Event} from 'sentry/types/event';
import type {OrganizationSummary} from 'sentry/types/organization';
import {trackAnalytics} from 'sentry/utils/analytics';
import {getDocsPlatform} from 'sentry/utils/docs';
import getDuration from 'sentry/utils/duration/getDuration';
Expand All @@ -35,6 +34,7 @@ import Projects from 'sentry/utils/projects';
const FRONTEND_PLATFORMS: string[] = [...frontend, ...mobile];
const BACKEND_PLATFORMS: string[] = [...backend, ...serverless];

import type {Organization} from 'sentry/types/organization';
import {generateLinkToEventInTraceView} from 'sentry/utils/discover/urls';

import {
Expand Down Expand Up @@ -67,7 +67,7 @@ type QuickTraceProps = Pick<
> & {
event: Event;
location: Location;
organization: OrganizationSummary;
organization: Organization;
quickTrace: QuickTraceType;
};

Expand Down Expand Up @@ -288,18 +288,14 @@ export default function QuickTrace({
return <QuickTraceContainer>{nodes}</QuickTraceContainer>;
}

function handleNode(key: string, organization: OrganizationSummary) {
function handleNode(key: string, organization: Organization) {
trackAnalytics('quick_trace.node.clicked', {
organization: organization.id,
node_key: key,
});
}

function handleDropdownItem(
key: string,
organization: OrganizationSummary,
extra: boolean
) {
function handleDropdownItem(key: string, organization: Organization, extra: boolean) {
const eventKey = extra
? 'quick_trace.dropdown.clicked_extra'
: 'quick_trace.dropdown.clicked';
Expand All @@ -316,7 +312,7 @@ type EventNodeSelectorProps = {
events: QuickTraceEvent[];
location: Location;
nodeKey: keyof typeof TOOLTIP_PREFIX;
organization: OrganizationSummary;
organization: Organization;
text: React.ReactNode;
traceSlug: string;
transactionDest: TransactionDestination;
Expand Down Expand Up @@ -410,10 +406,7 @@ function EventNodeSelector({
projectSlug: events[0].project_slug,
timestamp: events[0].timestamp,
location,
organization: {
slug: organization.slug,
features: organization.features,
},
organization,
transactionName: events[0].transaction,
type: transactionDest,
});
Expand Down Expand Up @@ -484,10 +477,7 @@ function EventNodeSelector({
projectSlug: event.project_slug,
eventId: event.event_id,
location,
organization: {
slug: organization.slug,
features: organization.features,
},
organization,
type: transactionDest,
transactionName: event.transaction,
});
Expand Down Expand Up @@ -525,7 +515,7 @@ function EventNodeSelector({
type DropdownNodeProps = {
anchor: 'left' | 'right';
event: TraceError | QuickTraceEvent | TracePerformanceIssue;
organization: OrganizationSummary;
organization: Organization;
allowDefaultEvent?: boolean;
onSelect?: (eventKey: any) => void;
subtext?: string;
Expand Down
12 changes: 6 additions & 6 deletions static/app/components/quickTrace/utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type {Location, LocationDescriptor} from 'history';
import {normalizeDateTimeParams} from 'sentry/components/organizations/pageFilters/parse';
import {ALL_ACCESS_PROJECTS} from 'sentry/constants/pageFilters';
import type {Event} from 'sentry/types/event';
import type {OrganizationSummary} from 'sentry/types/organization';
import type {Organization} from 'sentry/types/organization';
import {defined} from 'sentry/utils';
import EventView from 'sentry/utils/discover/eventView';
import {
Expand Down Expand Up @@ -32,7 +32,7 @@ export type TransactionDestination = 'discover' | 'performance';

export function generateIssueEventTarget(
event: TraceError | TracePerformanceIssue,
organization: OrganizationSummary,
organization: Organization,
referrer?: string
): LocationDescriptor {
const queryParams = referrer ? '?referrer=' + referrer : '';
Expand All @@ -41,7 +41,7 @@ export function generateIssueEventTarget(

function generateDiscoverEventTarget(
event: EventLite | TraceError | TracePerformanceIssue,
organization: OrganizationSummary,
organization: Organization,
location: Location,
referrer?: string
): LocationDescriptor {
Expand All @@ -67,7 +67,7 @@ function generateDiscoverEventTarget(

export function generateSingleErrorTarget(
event: TraceError | TracePerformanceIssue,
organization: OrganizationSummary,
organization: Organization,
location: Location,
destination: ErrorDestination,
referrer?: string
Expand All @@ -84,7 +84,7 @@ export function generateSingleErrorTarget(
export function generateMultiTransactionsTarget(
currentEvent: Event,
events: EventLite[],
organization: OrganizationSummary,
organization: Organization,
groupType: 'Ancestor' | 'Children' | 'Descendant'
): LocationDescriptor {
const queryResults = new MutableSearch([]);
Expand Down Expand Up @@ -134,7 +134,7 @@ export function getEventTimestamp(event: Event): string | number | undefined {

export function generateTraceTarget(
event: Event,
organization: OrganizationSummary,
organization: Organization,
location: Location,
source?: string
): LocationDescriptor {
Expand Down
9 changes: 8 additions & 1 deletion static/app/types/organization.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ export interface Organization extends OrganizationSummary {
allowJoinRequests: boolean;
allowSharedIssues: boolean;
attachmentsRole: string;
availableRoles: {id: string; name: string}[]; // Deprecated, use orgRoleList
/** @deprecated use orgRoleList instead. */
availableRoles: {id: string; name: string}[];
dataScrubber: boolean;
dataScrubberDefaults: boolean;
debugFilesRole: string;
Expand Down Expand Up @@ -79,6 +80,12 @@ export interface Organization extends OrganizationSummary {
trustedRelays: Relay[];
desiredSampleRate?: number | null;
effectiveSampleRate?: number | null;
extraOptions?: {
traces: {
checkSpanExtractionDate: boolean;
spansExtractionDate: number;
};
};
orgRole?: string;
planSampleRate?: number | null;
}
Expand Down
2 changes: 1 addition & 1 deletion static/app/utils/discover/urls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export function generateLinkToEventInTraceView({
}: {
eventId: string;
location: Location;
organization: Pick<Organization, 'slug' | 'features'>;
organization: Organization;
projectSlug: string;
timestamp: string | number;
traceSlug: string;
Expand Down
7 changes: 5 additions & 2 deletions static/app/views/performance/traceDetails/TraceDetailsRouting.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {browserHistory} from 'sentry/utils/browserHistory';
import {useLocation} from 'sentry/utils/useLocation';
import useOrganization from 'sentry/utils/useOrganization';

import {getTraceDetailsUrl} from './utils';
import {getTraceDetailsUrl, shouldForceRouteToOldView} from './utils';

type Props = {
children: JSX.Element;
Expand All @@ -25,7 +25,10 @@ function TraceDetailsRouting(props: Props) {
return children;
}

if (organization.features.includes('trace-view-v1')) {
if (
organization.features.includes('trace-view-v1') &&
!shouldForceRouteToOldView(organization, getEventTimestamp(event))
) {
if (event?.groupID && event?.eventID) {
const issuesLocation = `/organizations/${organization.slug}/issues/${event.groupID}/events/${event.eventID}`;
browserHistory.replace({
Expand Down
43 changes: 35 additions & 8 deletions static/app/views/performance/traceDetails/utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type {Location, LocationDescriptorObject} from 'history';

import {PAGE_URL_PARAM} from 'sentry/constants/pageFilters';
import type {Organization, OrganizationSummary} from 'sentry/types';
import type {Organization} from 'sentry/types';
import {getTimeStampFromTableDateField} from 'sentry/utils/dates';
import type {
EventLite,
Expand Down Expand Up @@ -29,7 +29,7 @@ export function getTraceDetailsUrl({
}: {
dateSelection;
location: Location;
organization: Pick<OrganizationSummary, 'slug' | 'features'>;
organization: Organization;
traceSlug: string;
demo?: string;
eventId?: string;
Expand All @@ -46,6 +46,17 @@ export function getTraceDetailsUrl({
[PAGE_URL_PARAM.PAGE_END]: end,
};

const oldTraceUrl = {
pathname: normalizeUrl(
`/organizations/${organization.slug}/performance/trace/${traceSlug}/`
),
query: queryParams,
};

if (shouldForceRouteToOldView(organization, timestamp)) {
return oldTraceUrl;
}

if (organization.features.includes('trace-view-v1')) {
if (spanId) {
queryParams.node = [`span-${spanId}`, `txn-${eventId}`];
Expand All @@ -68,12 +79,28 @@ export function getTraceDetailsUrl({
queryParams.limit = DEFAULT_TRACE_ROWS_LIMIT;
}

return {
pathname: normalizeUrl(
`/organizations/${organization.slug}/performance/trace/${traceSlug}/`
),
query: queryParams,
};
return oldTraceUrl;
}

/**
* Single tenant, on-premise etc. users may not have span extraction enabled.
*
* This code can be removed at the time we're sure all STs have rolled out span extraction.
*/
export function shouldForceRouteToOldView(
organization: Organization,
timestamp: string | number | undefined
) {
const usableTimestamp = getTimeStampFromTableDateField(timestamp);
if (!usableTimestamp) {
// Timestamps must always be provided for the new view, if it doesn't exist, fall back to the old view.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k-fish , it just helps make the query faster. The traceview still loads without it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize it works on the api but the frontend it should be mandatory no? it significantly improves trace performance, I had assumed all callsites were sending it where appropriate for the new view.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we do it from all callsites. However, even without it the new traceview should load faster than the old traceview I believe. But yeah, just wanted to point it out in case you thought that the new view wouldn't work at all without timestamp.

return true;
}

return (
organization.extraOptions?.traces.checkSpanExtractionDate &&
organization.extraOptions?.traces.spansExtractionDate <= usableTimestamp
);
}

function transactionVisitor() {
Expand Down
Loading