Skip to content

feat(discover): Zerofill queries that are grouped by time #11384

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
billyvg merged 6 commits into from
Jan 9, 2019
Merged

feat(discover): Zerofill queries that are grouped by time #11384

billyvg merged 6 commits into from
Jan 9, 2019

Conversation

@billyvg
Copy link
Member

@billyvg billyvg commented Jan 5, 2019

This will zerofill results when a query has the rollup parameter. This will give us more accurate charts when there are time periods without any events.

@billyvg
Copy link
Member Author

billyvg commented Jan 5, 2019

Do we want to have discover charts treat null values as 0?

@billyvg billyvg force-pushed the feat/discover/zerofill-time-rollups branch from 7dd78c5 to b6a83fc Compare January 7, 2019 22:01
billyvg
billyvg commented Jan 7, 2019
View reviewed changes
src/sentry/api/endpoints/organization_discover_query.py
result['project.name'] = projects[result['project.id']]
if 'project.id' not in requested_query['selected_columns']:
del result['project.id']
if 'project.id' in result:
Copy link
Member Author

@billyvg billyvg Jan 7, 2019

Choose a reason for hiding this comment

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

It's now possible this doesn't not exist in the result for time buckets that do not have any data.

@billyvg billyvg requested review from lynnagara and macqueen January 7, 2019 22:03
@billyvg billyvg force-pushed the feat/discover/zerofill-time-rollups branch from b6a83fc to 261ca47 Compare January 8, 2019 01:21
@lynnagara
Copy link
Member

lynnagara commented Jan 8, 2019

If we're zerofilling results for all queries with a rollup value provided, do we need the assumeEmptyAsZero flag on the UI at all?

@lynnagara
Copy link
Member

lynnagara commented Jan 8, 2019

Also, would this mean that limits are kind of ignored on these types of queries?

@billyvg
Copy link
Member Author

billyvg commented Jan 8, 2019

Can we bump the limit to 10k (that's what events is at)?

Limit is not ignored, you'll have a result object with {time: <ts>} and no other keys for every empty time bucket.

billyvg added 5 commits January 8, 2019 12:46
@billyvg
feat(discover): Zerofill queries that are grouped by time
a6fe10c 
This will zerofill results when a query has the `rollup` parameter. This will give us more accurate charts when there are time periods without any events.
@billyvg
fix to respect orderby
6f7382d
@billyvg
use larger rollup bucket
0a2e113
@billyvg
fix js tests
8140a93
@billyvg
change assumeEmptyAsZero behavior to be default
64df396
@billyvg billyvg force-pushed the feat/discover/zerofill-time-rollups branch from bd578c8 to 64df396 Compare January 8, 2019 20:50
@lynnagara
Copy link
Member

lynnagara commented Jan 9, 2019

I don't have a strong opinion on the API limit, but can we keep the one that user can enter in the UI as 1000?

@lynnagara
Copy link
Member

lynnagara commented Jan 9, 2019

I don't think I fully understand assumeNullAsZero - do we still need that and how is it distinct from assumeEmptyAsZero - are these not being zerofilled?

@billyvg
Copy link
Member Author

billyvg commented Jan 9, 2019

I removed the "assume as zero" option

@billyvg
Copy link
Member Author

billyvg commented Jan 9, 2019

I'm not going to touch limit for now, widgets use queryBuilder.fetch (where limit is validated) - so we'll have to think about where to validate it.

lynnagara
lynnagara approved these changes Jan 9, 2019
View reviewed changes
Copy link
Member

@lynnagara lynnagara left a comment

Choose a reason for hiding this comment

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

LGTM

@billyvg billyvg merged commit fa08187 into master Jan 9, 2019
@billyvg billyvg deleted the feat/discover/zerofill-time-rollups branch January 9, 2019 17:12
billyvg added a commit that referenced this pull request Jan 9, 2019
@billyvg
feat(discover): Zerofill queries that are grouped by time (#11384)
860ab93 
This will zerofill results when a query has the `rollup` parameter. This will give us more accurate charts when there are time periods without any events.
jan-auer added a commit that referenced this pull request Jan 10, 2019
@jan-auer
Merge branch 'master' into ref/query-string-pair-list
33c30bf 
* master: (56 commits)
  feat(issues) Add skeleton for Org wide issues (#11420)
  fix(api): Fix broken spam email blocking code
  don't need this white background anymore and it causes a bug on hosted setup (#11436)
  fix(charts): Fix max value for WorldMapChart (#11404)
  feat(issues): Add issues icon to sidebar (#11439)
  build: Remove 'exports' from sourcemaps sources prefix (#11438)
  fix: Render integration description as markdown in search (#11441)
  ref: Import jquery when it's used (#11430)
  fix(ui): Render message params (#11432)
  ref(releases): Refactor projects/organization release overview  (#11392)
  ref(groups): Refactor project group details (#11422)
  feat(2fa): Allow org to reset member 2fa (#11152)
  feat(api): Add relative stats period support to get_date_range_from_params (#11380)
  chore: Remove group-unmerge flag (#11431)
  fix(ui) Fix 'other' tag bucket to have a proper tooltip (#11433)
  build(dev): Add `yarn dev` script to start sentry devserver (#11360)
  ref(charts): Change PercentageBarChart -> PercentageAreaChart (#11401)
  feat(discover): Zerofill queries that are grouped by time (#11384)
  test: Add coverage for breadcrumb message scrubbing
  feat(releases): Add all organization release routes (#11377)
  ...
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lynnagara lynnagara lynnagara approved these changes

@macqueen macqueen Awaiting requested review from macqueen

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@billyvg @lynnagara