Skip to content
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.

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

feat(ui): Add new session category to the alert wizard #28590

Merged
merged 22 commits into from
Sep 17, 2021

Conversation

matejminar
Copy link
Member

image

Adding Session category to the alert wizard with two brand new options: Crash Free Session Rate and Crash Free User Rate.

It does not do anything yet, it's a feature flagged work in progress.

@matejminar matejminar requested a review from a team September 15, 2021 09:45
@matejminar matejminar requested a review from a team as a code owner September 15, 2021 09:45
@matejminar matejminar requested a review from a team September 15, 2021 09:45
/>
</OptionsWrapper>
))}
{alertWizardCategories(organization).map(
Copy link
Member

Choose a reason for hiding this comment

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

What about calling this getAlertWizardCategories?

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good, let me change that 👍

Comment on lines 77 to 82
if (org.features.includes('crash-rate-alerts')) {
options.splice(1, 0, {
categoryHeading: t('Sessions'),
options: ['crash_free_sessions', 'crash_free_users'],
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Would this be easier to read if it was part of the options declaration? That way you'd be able to see the intended ordering instead of having to parse the splice() call.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, splice is never easy to parse.

{i > 0 && <Styledh2>{categoryHeading}</Styledh2>}
<RadioPanelGroup
choices={options.map(alertType => {
return [alertType, AlertWizardAlertNames[alertType]];
Copy link
Member

Choose a reason for hiding this comment

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

nit: should this const have the first letter lower case?

Suggested change
return [alertType, AlertWizardAlertNames[alertType]];
return [alertType, alertWizardAlertNames[alertType]];

Copy link
Member Author

Choose a reason for hiding this comment

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

there are lots of constants like this in that file, I didn't want to go on a refactoring spree because this PR would get messy

{getAlertWizardCategories(organization).map(
({categoryHeading, options}, i) => (
<OptionsWrapper key={categoryHeading}>
{i > 0 && <Styledh2>{categoryHeading}</Styledh2>}
Copy link
Member

Choose a reason for hiding this comment

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

nit pick:

Suggested change
{i > 0 && <Styledh2>{categoryHeading}</Styledh2>}
{i > 0 && <StyledH2>{categoryHeading}</StyledH2>}

Copy link
Member Author

Choose a reason for hiding this comment

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

If we want to refactor this, I'll change it to CategoryTitle so that it's more descriptive. 👍

Co-authored-by: Priscila Oliveira <priscilawebdev@gmail.com>
loewenheim and others added 10 commits September 17, 2021 12:03
…IVE-209) (#28593)

This adds a field in the project details which exposes the health of
the project's symbolication processing.  The aim is to let the UI show
warnings on the project if we are experiencing processing problems.

Currently returns dummy data, but a followup will change this to read
from 3 sentry options to return the correct value.

NATIVE-209
Follow up to #28382 to remove the endpoints as they won't be in use anymore.
Adding the ability to unfurl multi y-axis charts.
* add default business for all experiment

* change name
…s rather than semver cols (#28622)

Previously a search like `1.2` would be converted internally to `1.2.*`. This excludes any versions
like `1.20`, `1.200`, etc. To make this work as the user expects, instead we now just use string
prefixing. This should work in most cases, but if there are versions with 0 prefixes then those will
need to be included in the search string for it to work. So for example, `1.020.0` won't show up if
someone types `1.2`, only if they type `1.`, `1.0`, etc.

We still sort these results using semver sort, so they're returned in the order that the user
expects.
ref(ui): Add performance event view hook

This adds a hook to avoid having to constantly drill event view, especially for our generic discover query specializations. Essentially if any specialization now uses the 'DiscoverQueryPropsWithContext' type you no longer have to provide eventView and orgSlug, which should reduce a lot of boilerplate throughout performance. It's not on by default since there is likely some undefined checks and tests that might need to be rewritten (to use a provider) if you choose to use this.

Other:
- Giving a context wrapper to always check if a context is defined, we'll see if it's helpful, and if so we can raise this up to be used everywhere in the UI or talk about it during TSC.
markstory and others added 6 commits September 17, 2021 12:03
We have a few organization that have 0 repositories, but still have
commits. Make CommitAuthor deletions record based so we can also remove
commits that would block a commit author deletion.

Fixes SENTRY-S5Y
We had two different implementations of group deletion. This change
consolidates those into a single path and fixes split group hashes being
removed by list page delete operations.

I've also added the organization_id to group deletion messages. This
helps improve log queries like 'did acme org delete any issues recently'
which comes up when customers notice discrepancies between the events
they currently have and the usage we've tracked.
Changes from legacy react-select choices to options in step 2 and step 5 of this modal
New mediator handles the execution of webhooks based on an alert rule triggering.
Bonus: Added some logging for easier debugging in the future

Fixes API-2075
…re emits updates (#28610)

Currently, the <Projects> util component does not react to updates from the ProjectsStore. This means there can be instances when project data has changed, but the application doesn't seem to reflect it. One such example is creating an issue alert or metric alert. Adding a team to a project should affect the state of both of these components as adding a team to a project in one dropdown should enable it in the other dropdown.
@matejminar matejminar merged commit ef5c09d into master Sep 17, 2021
@matejminar matejminar deleted the feat/session-alert-categories branch September 17, 2021 10:59
@github-actions github-actions bot locked and limited conversation to collaborators Oct 2, 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.