chore(ts): Improve Starfish span metrics typing #53453

gggritso · 2023-07-24T18:42:25Z

Still a lot of cleanup to do, but this takes a few initial steps.

Remove unused Span type. Literally not used anywhere
Remove IndexedSpan type. This one was a little silly. It was used in a few places, mostly to pick off the group and op properties. Wrong for a bunch of reasons, but mostly wrong because SpanIndexedFieldTypes covers the same space but much better.
Improve SpanIndexedFieldTypes by adding a few missing fields
Add SpanMetricsFieldTypes to match the indexed version
Remove newly unused useSpanMeta hook

Also, a bunch of places were accepting a span, but it's just an object with a group. I replaced it with a plain string parameter. In the future, I'll probably just merge it into queryFilters, since group is a filter like any other.

Next up I'll probably consolidate some of these field names, and start looking at adding more descriptive, strict typing.

It's pointless to pass a "partial" span, and use the `IndexedSpan` type.

This was a duplicate of a type defined elsewhere.

AbhiPrasad

good change, feel free to use my review comments however you want.

We probably should add a validation step on initial query so we can validate if data is returned in the expected schema (can just send an error to Sentry), that way we can detect fe/be problems faster (validation can even happen in a useEffect so it doesn't block).

AbhiPrasad · 2023-07-24T19:18:17Z

static/app/views/starfish/queries/useSpanMetricsSeries.tsx

  queryFilters: SpanSummaryQueryFilters,
  yAxis: string[] = [],
  referrer = 'span-metrics-series'
 ) => {
  const pageFilters = usePageFilters();

-  const eventView = span
-    ? getEventView(span, pageFilters.selection, yAxis, queryFilters)
+  const eventView = group


m: How can group be undefined here if it's defined as string in useSpanMetricsSeries? We can remove the Boolean(group) cast too then.

True, but in Step 2 I'm going to move the group parameter into queryFilters, since it's just a filter like anything else, so this code isn't long for this world

AbhiPrasad · 2023-07-24T19:22:04Z

static/app/views/starfish/types.tsx

@@ -26,6 +26,17 @@ export enum SpanMetricsFields {
  SPAN_SELF_TIME = 'span.self_time',
 }

+export type SpanMetricsFieldTypes = {


I think we want to prefer interfaces whenever possible due to https://github.com/microsoft/TypeScript/wiki/Performance#preferring-interfaces-over-intersections

Is it completely whenever possible? I thought we were mostly using it for things like props that get extended. I'm happy to change in a future iteration, though!

I've just been defaulting to interface, but I think we can do this whenever

AbhiPrasad · 2023-07-24T19:25:40Z

static/app/views/starfish/views/spanSummaryPage/index.tsx

      'time_spent_percentage()',
      'http_error_count()',
    ],
    'api.starfish.span-summary-page-metrics'
  );

-  const span = Object.assign({group: groupId}, spanMetrics as SpanMetrics & SpanMeta);
+  const span = {


l: Isn't this more accurate?

Suggested change

const span = {

const span: SpanMetricsFieldTypes = {

Neither is fully accurate unfortunately. Another down-the-road fix that I want is for API calls to correctly type their return values based on the fields that were passed in, but one thing at a time

AbhiPrasad · 2023-07-24T19:26:38Z

static/app/views/starfish/views/spanSummaryPage/index.tsx

      'api.starfish.span-summary-page-metrics-chart'
    );

  useSynchronizeCharts([!areSpanMetricsSeriesLoading]);

  const spanMetricsThroughputSeries = {
-    seriesName: span?.[SPAN_OP]?.startsWith('db') ? 'Queries' : 'Requests',
+    seriesName: span?.[SpanMetricsFields.SPAN_OP]?.startsWith('db')


Suggested change

seriesName: span?.[SpanMetricsFields.SPAN_OP]?.startsWith('db')

seriesName: span[SpanMetricsFields.SPAN_OP]?.startsWith('db')

span should always be defined here right?

l: We can then think about extracting span[SpanMetricsFields.SPAN_OP] into it's own var.

Yep, always defined! We sprinkled ?. everywhere else in the file, so I didn't bother cleaning that up for now. I wonder if TypeScript can detect redundant optional lookups 🤔

there is https://typescript-eslint.io/rules/no-unnecessary-condition/, perhaps we should turn it on

AbhiPrasad · 2023-07-24T19:28:03Z

static/app/views/starfish/queries/useSpanTransactionMetrics.tsx

@@ -26,35 +25,33 @@ export type SpanTransactionMetrics = {
 };

 export const useSpanTransactionMetrics = (
-  span: Pick<IndexedSpan, 'group'>,
+  group: string,


m: should we rename to either group or groupId across the board?

It should be group everywhere, I just chose not to do it in this particular PR

gggritso · 2023-07-24T20:04:15Z

@AbhiPrasad right on, thanks for the comments! Overall, all fair and I'll be addressing them all in some shape in upcoming PRs this week and next. Just trying to keep the changes isolated, so the PRs are abit "incomplete" while I look for manageable chunks to fix

AbhiPrasad · 2023-07-25T13:48:53Z

Small PRs best PRs

Still a lot of cleanup to do, but this takes a few initial steps. 1. Remove unused `Span` type. Literally not used anywhere 2. Remove `IndexedSpan` type. This one was a little silly. It was used in a few places, mostly to pick off the `group` and `op` properties. Wrong for a bunch of reasons, but mostly wrong because `SpanIndexedFieldTypes` covers the same space but much better. 3. Improve `SpanIndexedFieldTypes` by adding a few missing fields 4. Add `SpanMetricsFieldTypes` to match the indexed version 5. Remove newly unused `useSpanMeta` hook Also, a bunch of places were accepting a `span`, but it's just an object with a group. I replaced it with a plain string parameter. In the future, I'll probably just merge it into `queryFilters`, since `group` is a filter like any other. Next up I'll probably consolidate some of these field names, and start looking at adding more descriptive, strict typing.

gggritso added 5 commits July 24, 2023 11:58

Remove unused Span type

13772a6

Improve usage of IndexedSpan

a0445bf

It's pointless to pass a "partial" span, and use the `IndexedSpan` type.

Remove unnecessary indexed span type

c38044f

This was a duplicate of a type defined elsewhere.

Fix typing

24466d1

Remove unused file

c6411f4

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jul 24, 2023

Merge branch 'master' into ref/starfish/remove-redundant-types

0830c54

vercel bot deployed to Preview July 24, 2023 18:47 View deployment

gggritso marked this pull request as ready for review July 24, 2023 19:12

gggritso requested a review from a team July 24, 2023 19:12

AbhiPrasad approved these changes Jul 24, 2023

View reviewed changes

gggritso merged commit ccb55e8 into master Jul 25, 2023

gggritso deleted the ref/starfish/remove-redundant-types branch July 25, 2023 13:27

github-actions bot locked and limited conversation to collaborators Aug 10, 2023

	seriesName: span?.[SpanMetricsFields.SPAN_OP]?.startsWith('db')
	seriesName: span[SpanMetricsFields.SPAN_OP]?.startsWith('db')

Uh oh!

chore(ts): Improve Starfish span metrics typing #53453

chore(ts): Improve Starfish span metrics typing #53453

Uh oh!

Conversation

gggritso commented Jul 24, 2023

Uh oh!

AbhiPrasad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gggritso commented Jul 24, 2023

Uh oh!

AbhiPrasad commented Jul 25, 2023

Uh oh!

Uh oh!