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.

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

Revisit custom threshold types in public folder #170306

Merged
merged 30 commits into from
Nov 14, 2023
Merged

Revisit custom threshold types in public folder #170306

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
f0b93a7
Rename metricThreshold to customThreshold in public
maryam-saeidi Oct 26, 2023
000696c
Merge branch 'main' into 159340-revisit-custom-threshold-public-types
maryam-saeidi Oct 27, 2023
babe850
Rename metricThreshold to customThreshold in public folder
maryam-saeidi Oct 27, 2023
7385077
Rename metricThreshold and remove unused types
maryam-saeidi Oct 27, 2023
b23afdf
Remove extra logic related to aggregations other than custom for defa…
maryam-saeidi Oct 27, 2023
4b6b1fa
Remove metric_expression_params functions
maryam-saeidi Oct 30, 2023
cf8e0f6
Remove extra validation for aggType custom
maryam-saeidi Oct 30, 2023
84ded2f
Remove preFill logic and unnecessary types
maryam-saeidi Nov 1, 2023
f79a012
Merge branch 'main' into 159340-revisit-custom-threshold-public-types
maryam-saeidi Nov 1, 2023
881a6c1
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine Nov 1, 2023
886a4ed
Fix pct field format and rearrange types
maryam-saeidi Nov 1, 2023
9b3f22e
Merge branch 'main' into 159340-revisit-custom-threshold-public-types
maryam-saeidi Nov 1, 2023
c69f7b0
Fix no data reason and refactor threshold component
maryam-saeidi Nov 1, 2023
4d6e9f0
Merge branch 'main' into 159340-revisit-custom-threshold-public-types
maryam-saeidi Nov 1, 2023
c4e25e8
Fix unit props
maryam-saeidi Nov 1, 2023
cd73bae
Merge branch 'main' into 159340-revisit-custom-threshold-public-types
maryam-saeidi Nov 1, 2023
d2417fc
Remove aggregators that are not supported in the custom threshold
maryam-saeidi Nov 2, 2023
922c069
Merge branch 'main' into 159340-revisit-custom-threshold-public-types
maryam-saeidi Nov 2, 2023
a298924
Remove more types
maryam-saeidi Nov 2, 2023
5ef6a4c
Remove usage of Aggregators.CUSTOM
maryam-saeidi Nov 2, 2023
de870f6
Merge branch 'main' into 159340-revisit-custom-threshold-public-types
maryam-saeidi Nov 2, 2023
2bcb226
Merge branch 'main' into 159340-revisit-custom-threshold-public-types
maryam-saeidi Nov 8, 2023
6debb6f
Remove comment
maryam-saeidi Nov 8, 2023
d630f20
Fix translation
maryam-saeidi Nov 8, 2023
1e7ea5d
Merge branch 'main' into 159340-revisit-custom-threshold-public-types
fkanout Nov 9, 2023
7bbca06
Merge branch 'main' into 159340-revisit-custom-threshold-public-types
maryam-saeidi Nov 11, 2023
261685c
Fix bug in changing aggs
maryam-saeidi Nov 13, 2023
a24ebd6
Merge branch 'main' into 159340-revisit-custom-threshold-public-types
maryam-saeidi Nov 13, 2023
e02debb
Merge branch 'main' into 159340-revisit-custom-threshold-public-types
maryam-saeidi Nov 13, 2023
a91dc5e
Merge branch 'main' into 159340-revisit-custom-threshold-public-types
maryam-saeidi Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion x-pack/packages/observability/alerting_test_data/src/create_custom_threshold_rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { CUSTOM_AGGREGATOR } from '@kbn/observability-plugin/common/custom_threshold_rule/constants';
import {
Aggregators,
Comparator,
Expand Down Expand Up @@ -38,7 +39,7 @@ export const createCustomThresholdRule = async (
params: {
criteria: ruleParams.params?.criteria || [
{
aggType: Aggregators.CUSTOM,
aggType: CUSTOM_AGGREGATOR,
comparator: Comparator.GT,
threshold: [1],
timeSize: 1,
Expand Down
3 changes: 2 additions & 1 deletion x-pack/packages/observability/alerting_test_data/src/scenarios/custom_threshold_log_count.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { CUSTOM_AGGREGATOR } from '@kbn/observability-plugin/common/custom_threshold_rule/constants';
import {
Aggregators,
Comparator,
Expand All @@ -22,7 +23,7 @@ export const scenario1 = {
params: {
criteria: [
{
aggType: Aggregators.CUSTOM,
aggType: CUSTOM_AGGREGATOR,
comparator: Comparator.LT,
threshold: [100],
timeSize: 1,
Expand Down
3 changes: 2 additions & 1 deletion ...ages/observability/alerting_test_data/src/scenarios/custom_threshold_log_count_groupby.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { CUSTOM_AGGREGATOR } from '@kbn/observability-plugin/common/custom_threshold_rule/constants';
import {
Aggregators,
Comparator,
Expand All @@ -22,7 +23,7 @@ export const scenario2 = {
params: {
criteria: [
{
aggType: Aggregators.CUSTOM,
aggType: CUSTOM_AGGREGATOR,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same: I think we should keep referring to the custom from Aggregators to keep everything on track when an updates occurs. Can you explain the benefits of not using the value fromAggregators?
I will not repeate the same comment for the other files :D

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 changed it to CUSTOM_AGGREGATOR because it is the only acceptable value at this level and does not belong to Aggregators anymore. There were a lot of type duplications to handle this scenario, and by splitting them, we don't need to exclude custom from Aggregators to be used in the rule.

I am considering creating a ticket to remove CUSTOM_AGGREGATOR everywhere if it does not have a backward compatibility issue.

comparator: Comparator.LT,
threshold: [40],
timeSize: 1,
Expand Down
3 changes: 2 additions & 1 deletion ...kages/observability/alerting_test_data/src/scenarios/custom_threshold_log_count_nodata.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { CUSTOM_AGGREGATOR } from '@kbn/observability-plugin/common/custom_threshold_rule/constants';
import {
Aggregators,
Comparator,
Expand All @@ -22,7 +23,7 @@ export const scenario3 = {
params: {
criteria: [
{
aggType: Aggregators.CUSTOM,
aggType: CUSTOM_AGGREGATOR,
comparator: Comparator.LT,
threshold: [5],
timeSize: 1,
Expand Down
3 changes: 2 additions & 1 deletion ...ck/packages/observability/alerting_test_data/src/scenarios/custom_threshold_metric_avg.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { CUSTOM_AGGREGATOR } from '@kbn/observability-plugin/common/custom_threshold_rule/constants';
import {
Aggregators,
Comparator,
Expand All @@ -22,7 +23,7 @@ export const scenario4 = {
params: {
criteria: [
{
aggType: Aggregators.CUSTOM,
aggType: CUSTOM_AGGREGATOR,
comparator: Comparator.GT,
threshold: [80],
timeSize: 1,
Expand Down
3 changes: 2 additions & 1 deletion ...ges/observability/alerting_test_data/src/scenarios/custom_threshold_metric_avg_groupby.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { CUSTOM_AGGREGATOR } from '@kbn/observability-plugin/common/custom_threshold_rule/constants';
import {
Aggregators,
Comparator,
Expand All @@ -22,7 +23,7 @@ export const scenario5 = {
params: {
criteria: [
{
aggType: Aggregators.CUSTOM,
aggType: CUSTOM_AGGREGATOR,
comparator: Comparator.GT,
threshold: [80],
timeSize: 5,
Expand Down
3 changes: 2 additions & 1 deletion ...ages/observability/alerting_test_data/src/scenarios/custom_threshold_metric_avg_nodata.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { CUSTOM_AGGREGATOR } from '@kbn/observability-plugin/common/custom_threshold_rule/constants';
import {
Aggregators,
Comparator,
Expand All @@ -22,7 +23,7 @@ export const scenario6 = {
params: {
criteria: [
{
aggType: Aggregators.CUSTOM,
aggType: CUSTOM_AGGREGATOR,
comparator: Comparator.LT,
threshold: [1],
timeSize: 1,
Expand Down
7 changes: 2 additions & 5 deletions x-pack/plugins/observability/common/custom_threshold_rule/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,13 @@
* 2.0.
*/

export const SNAPSHOT_CUSTOM_AGGREGATIONS = ['avg', 'max', 'min', 'rate'] as const;
export const METRIC_EXPLORER_AGGREGATIONS = [
Copy link
Contributor

Choose a reason for hiding this comment

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

To be deleted it's not used anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, this is still used in x-pack/plugins/observability/server/lib/rules/custom_threshold/register_custom_threshold_rule_type.ts

'avg',
'max',
'min',
'cardinality',
'rate',
'count',
'sum',
'p95',
'p99',
'custom',
] as const;

export const CUSTOM_AGGREGATOR = 'custom';
2 changes: 1 addition & 1 deletion x-pack/plugins/observability/common/custom_threshold_rule/formatters/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { InfraWaffleMapDataFormat } from './types';

export const FORMATTERS = {
number: formatNumber,
// Because the implimentation for formatting large numbers is the same as formatting
// Because the implementation for formatting large numbers is the same as formatting
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

// bytes we are re-using the same code, we just format the number using the abbreviated number format.
abbreviatedNumber: createBytesFormatter(InfraWaffleMapDataFormat.abbreviatedNumber),
// bytes in bytes formatted string out
Expand Down
154 changes: 0 additions & 154 deletions x-pack/plugins/observability/common/custom_threshold_rule/metrics_explorer.ts
View file
Open in desktop

This file was deleted.

Loading