Skip to content

Conversation

@edwardgou-sentry
Copy link
Contributor

Allow Dashboard Top Events widgets to add aggregate columns so that chart can be sorted by aggregates

image

@edwardgou-sentry edwardgou-sentry requested a review from a team November 1, 2021 19:44
@edwardgou-sentry edwardgou-sentry marked this pull request as ready for review November 1, 2021 20:52
@edwardgou-sentry edwardgou-sentry requested a review from a team as a code owner November 1, 2021 20:52
@edwardgou-sentry edwardgou-sentry marked this pull request as draft November 1, 2021 20:56
@edwardgou-sentry edwardgou-sentry marked this pull request as ready for review November 1, 2021 20:58
@github-actions
Copy link
Contributor

github-actions bot commented Nov 1, 2021

size-limit report

Path Base Size (16e9dcd) Current Size Change
src/sentry/static/sentry/dist/entrypoints/app.js 52.76 KB 52.76 KB -0.01% 🔽
src/sentry/static/sentry/dist/entrypoints/sentry.css 70.89 KB 70.89 KB 0%

const fieldStr = generateFieldAsString(field);
return isAggregateField(fieldStr) || isAggregateEquation(fieldStr);
}) as QueryFieldValue;
const columns = fields.filter(field => field !== fieldValue);
Copy link
Member

@shruthilayaj shruthilayaj Nov 4, 2021

Choose a reason for hiding this comment

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

So basically we're saying that the first aggregate field from the bottom up is the y-axis? Can we do something simpler instead like:
const fieldValue = fields[-1]
const columns = fields.slice(0, fields.length -1)

Would that work? because it's a single yAxis that is always required and populated for ton events?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that would work! Updated to this, tested and cleaned up some other convoluted parts of code (don't recall why I made it so complicated lol). Also fixed a bug with default query field.

Copy link
Member

@shruthilayaj shruthilayaj left a comment

Choose a reason for hiding this comment

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

Something funky is going on when you save the widget/dashboard. Steps to replicate:

  1. Add title and eps() as columns
  2. count() as the yaxis
  3. orderby count()
  4. save widget and dashboard
  5. open widget edit again, columns and y-axis aren't being saved as expected

@edwardgou-sentry
Copy link
Contributor Author

Something funky is going on when you save the widget/dashboard. Steps to replicate:

  1. Add title and eps() as columns
  2. count() as the yaxis
  3. orderby count()
  4. save widget and dashboard
  5. open widget edit again, columns and y-axis aren't being saved as expected

Good catch, I was able to replicated. Made an update to normalizeQueries for TOP_N to fix! Behaviour will be the same as TABLE now (it was removing some fields before).

Copy link
Member

@shruthilayaj shruthilayaj left a comment

Choose a reason for hiding this comment

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

Looks great! Loving the overall simplification!

return queries;
}

if (displayType === DisplayType.TOP_N) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: just combine with the if DisplayType.TABLE statement above

@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants