Skip to content

fix(timeseries): Add a check that the other series has data#101476

Merged
wmak merged 4 commits into
masterfrom
wmak/fix/add-test-for-None-other
Oct 16, 2025
Merged

fix(timeseries): Add a check that the other series has data#101476
wmak merged 4 commits into
masterfrom
wmak/fix/add-test-for-None-other

Conversation

@wmak

@wmak wmak commented Oct 14, 2025

Copy link
Copy Markdown
Member
  • This attempts to fix a bug where the other series was being included even when it has no data
  • Fixes the order of the other series when there's less than topEvents timeseries
  • Changes the top events groupby to have a None rather than the string None when there's a None value

- There was seemingly a mismatch of stats vs timeseries behaviour when
  the groupby value was None, this adds a test to confirm that
  timeseries is behaving as expected
- This adds a check if the other series has data before adding it
- I'm HOPING this will fix a prod vs local diff where production still
  returns the other series even when it doesn't have data
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 15, 2025
cursor[bot]

This comment was marked as outdated.

@codecov

codecov Bot commented Oct 15, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/snuba/discover.py 75.00% 1 Missing ⚠️
src/sentry/snuba/rpc_dataset_common.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #101476      +/-   ##
===========================================
+ Coverage   80.42%    80.81%   +0.39%     
===========================================
  Files        8701      8709       +8     
  Lines      386148    388101    +1953     
  Branches    24409     24409              
===========================================
+ Hits       310546    313649    +3103     
+ Misses      75251     74101    -1150     
  Partials      351       351              

@wmak wmak changed the title fix(timeseries): Add test for when a field has None groupby fix(timeseries): Add a check that the other series has data Oct 16, 2025
@wmak wmak requested a review from a team as a code owner October 16, 2025 17:28
@wmak wmak enabled auto-merge (squash) October 16, 2025 17:28
if row[axis] and row[axis] != 0:
return True
return False

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: Timeseries Data Missing Expected Axis

In check_timeseries_has_data, accessing row[axis] directly can raise a KeyError if a row in the timeseries data is missing an expected axis from the y_axes list.

Fix in Cursor Fix in Web

@wmak wmak merged commit 76ada0f into master Oct 16, 2025
68 checks passed
@wmak wmak deleted the wmak/fix/add-test-for-None-other branch October 16, 2025 17:49
@github-actions github-actions Bot locked and limited conversation to collaborators Nov 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants