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

fix(alerts): Metric alert rule save error msg when missing rule name #34729

Merged
merged 2 commits into from May 18, 2022

Conversation

taylangocmen
Copy link
Contributor

This restores the control state to alert rule name in the metric alert rule editor. Updates the error message with the missing fields. Also removes the ms from the Cumulative Layout Shift error trigger thresholds.

V3 metric example:
v3-metric

non-V3 metric example:
non-v3-metric

Error messages:
Screen Shot 2022-05-17 at 9 22 35 PM
Screen Shot 2022-05-17 at 9 22 46 PM
Screen Shot 2022-05-17 at 9 23 08 PM

@taylangocmen taylangocmen requested review from a team and dashed May 18, 2022 04:24
@@ -157,6 +157,11 @@ class TriggerFormContainer extends Component<TriggerFormContainerProps> {
};

getThresholdUnits(aggregate: string, comparisonType: AlertRuleComparisonType) {
// cls is a number not a measurement of time
if (aggregate.includes('measurements.cls')) {
Copy link
Member

Choose a reason for hiding this comment

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

@taylangocmen Using the measurementType() function might be better as we add more non-duration based measurements to alerts in the future (e.g. Mobile Vitals):

export function measurementType(field: string): MeasurementType {
if (MEASUREMENTS.hasOwnProperty(field)) {
return MEASUREMENTS[field];
}
return 'number';
}

Suggested change
if (aggregate.includes('measurements.cls')) {
if (measurementType(aggregate) !== 'duration') {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just measurementType(aggregate) would ignore the non-vital type aggregates like transaction.duration, so a combination of the two is needed

Copy link
Member

Choose a reason for hiding this comment

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

@taylangocmen Ah. In that case, aggregateFunctionOutputType() might be an option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually measurementType(aggregate) wouldn't work on alert MEASUREMENTS fields because the alert aggregate includes the aggregate function ie. percentile(measurements.lcp,0.5)' or p99(measurements.lcp) vs just measurements.lcp substring matching on every aggregate here isn't worth it.

@taylangocmen taylangocmen requested a review from dashed May 18, 2022 19:56
@taylangocmen taylangocmen merged commit c664d1d into master May 18, 2022
@taylangocmen taylangocmen deleted the taylangocmen/alert-rule-name-missing-error-msg branch May 18, 2022 22:41
@github-actions github-actions bot locked and limited conversation to collaborators Jun 3, 2022
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.

None yet

3 participants