Skip to content

chore(timeseries): Improve comparison logic #101194

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

Jump to bottom
Merged
merged 6 commits into from
Oct 8, 2025
Merged

chore(timeseries): Improve comparison logic #101194

merged 6 commits into from
Oct 8, 2025

Conversation

gggritso
Copy link
Member

@gggritso gggritso commented Oct 8, 2025

A few more improvements to the comparison algorith. This loosens the comparison criteria somewhat, and improves some of the behaviour.

  • Improve groupBy type and docs
  • Improve groupBy parsing
  • Add support for logs
  • Ignore dataScanned in comparator
  • Apply loose comparison to all numeric keys

@gggritso gggritso requested review from a team as code owners October 8, 2025 19:37
@gggritso gggritso requested a review from DominikB2014 October 8, 2025 19:37
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Oct 8, 2025
DominikB2014
DominikB2014 approved these changes Oct 8, 2025
View reviewed changes
Copy link
Contributor

@DominikB2014 DominikB2014 left a comment

Choose a reason for hiding this comment

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

Lgtm! I like this effort in getting all these types to be 100% accurate too

static/app/views/insights/common/queries/useSortedTimeSeries.tsx
Comment on lines +124 to +125
const timeSeriesResult = useFetchEventsTimeSeries(
dataset ?? DiscoverDatasets.SPANS,
Copy link
Contributor

Choose a reason for hiding this comment

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

I kinda liked how we used to have a hook for each dataset. We could alternatively do useFetchSpanTImeSeries, and useFetchLogTimeSeries. I like the little abstraction, imo in code your likely thinking "i wanna fetch a span time series", not "I wanna fetch an events timeseries", although usability wise its pretty much the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

We still have the hook for each dataset! useSortedTimeSeries uses the type-less one though, because it's used for both logs and traces

@codecov Codecov
Copy link

codecov bot commented Oct 8, 2025 •
edited
Loading

❌ 2 Tests Failed:

Tests completed Failed Passed Skipped
12144 2 12142 10
View the full list of 2 ❄️ flaky test(s) 
parseGroupBy handles empty fields array

Flake rate in main: 5.88% (Passed 16 times, Failed 1 times)

Stack Traces | 0.004s run time 
Error: expect(received).toBeUndefined()

Received: null
    at Object.toBeUndefined (.../utils/timeSeries/parseGroupBy.spec.tsx:50:20)
    at Promise.finally.completed (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:1559:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:1499:10)
    at _callCircusTest (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:1009:40)
    at processTicksAndRejections (node:internal/process/task_queues:105:5)
    at _runTest (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:949:3)
    at _runTestsForDescribeBlock (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:839:13)
    at _runTestsForDescribeBlock (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:829:11)
    at run (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:757:3)
    at runAndTransformResultsToJestFormat (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:1920:21)
    at jestAdapter (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/runner.js:101:19)
    at runTestInternal (.../sentry/node_modules/.pnpm/jest-runner@30.0..../jest-runner/build/testWorker.js:272:16)
    at runTest (.../sentry/node_modules/.pnpm/jest-runner@30.0..../jest-runner/build/testWorker.js:340:7)
    at Object.worker (.../sentry/node_modules/.pnpm/jest-runner@30.0..../jest-runner/build/testWorker.js:494:12)
parseGroupBy returns undefined for "Other" group name

Flake rate in main: 5.88% (Passed 16 times, Failed 1 times)

Stack Traces | 0.015s run time 
Error: expect(received).toBeUndefined()

Received: null
    at Object.toBeUndefined (.../utils/timeSeries/parseGroupBy.spec.tsx:6:20)
    at Promise.finally.completed (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:1559:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:1499:10)
    at _callCircusTest (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:1009:40)
    at processTicksAndRejections (node:internal/process/task_queues:105:5)
    at _runTest (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:949:3)
    at _runTestsForDescribeBlock (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:839:13)
    at _runTestsForDescribeBlock (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:829:11)
    at run (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:757:3)
    at runAndTransformResultsToJestFormat (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/jestAdapterInit.js:1920:21)
    at jestAdapter (.../sentry/node_modules/.pnpm/jest-circus@30.0.4_babel-plugin-macros@3.1..../jest-circus/build/runner.js:101:19)
    at runTestInternal (.../sentry/node_modules/.pnpm/jest-runner@30.0..../jest-runner/build/testWorker.js:272:16)
    at runTest (.../sentry/node_modules/.pnpm/jest-runner@30.0..../jest-runner/build/testWorker.js:340:7)
    at Object.worker (.../sentry/node_modules/.pnpm/jest-runner@30.0..../jest-runner/build/testWorker.js:494:12)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@gggritso gggritso merged commit 361c184 into master Oct 8, 2025
46 checks passed
@gggritso gggritso deleted the chore/timeseries/improve-conversion-again branch October 8, 2025 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DominikB2014 DominikB2014 DominikB2014 approved these changes

Assignees

No one assigned

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gggritso @DominikB2014