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(slo): allow configuration of advanced settings from UI #200822

Merged
merged 28 commits into from
Dec 2, 2024

Conversation

kdelemme
Copy link
Contributor

@kdelemme kdelemme commented Nov 19, 2024

Resolves #200041

🐎 Summary

This PR adds an advanced settings section at the end of the objective section in the SLO form. This section can be used to configure the following settings fields: syncField, syncDelay, frequency and preventInitialBackfill.

syncField is a new settings. The other fields were already defined at the API level.
Except for preventInitialBackfill, the other fields were not available on the form.

syncField is optional, therefore current usage of the create/update APIs won't break if this field is not specified. A null value is used to specifically unset the field when updating an SLO.

image

I've also taken the opportunity to close #194957 by fixing the index row label state when the form is invalid.

image

Note

While implementing this feature, I've noticed some bugs in the form state when changing the DataView / selected index. The dependant fields like groupBy and the new syncField were not cleared from the previously selected values, but the UI was hidding them.

The form was keeping the previous selected fields in its state, while hiding them from the combobox selection. This could lead to subtle bugs where user would edit an existing SLO, changing the DataView and hitting save, thinking the groupBy/syncField were cleared.

I've fixed this in this PR as well.

🧬 Testing

  • Creating an SLO should:
    • Use the syncField, syncDelay, Frequency provided values to build the rollup transform
    • Use default values if not provided
  • Editing an SLO should
    • Use the syncField, syncDelay, Frequency provided values to build the rollup transform
    • Keep the previous settings if not overridden

Release Note

  • Adds syncField, syncDelay and frequency settings in the API and SLO form. Allow customer to fine tune their SLO settings directly from the UI.

@kdelemme kdelemme force-pushed the feat/advanced-settings-slo branch from 0c6c303 to e3011a8 Compare November 22, 2024 13:58
@@ -42,7 +42,7 @@ export class ApmTransactionDurationTransformGenerator extends TransformGenerator
this.buildDestination(slo),
this.buildGroupBy(slo, slo.indicator),
this.buildAggregations(slo, slo.indicator),
this.buildSettings(slo),
this.buildSettings(slo, '@timestamp'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

APM always uses @timestamp

Comment on lines +159 to +162
settings: merge(
{ preventInitialBackfill: false, syncDelay: '1m', frequency: '1m' },
storedSLO.settings
),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

merge() simplifies the backward compatibility management of the settings

Comment on lines +207 to +214
settings: merge(
{
syncDelay: new Duration(1, DurationUnit.Minute),
frequency: new Duration(1, DurationUnit.Minute),
preventInitialBackfill: false,
},
params.settings
),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We set default values, and override with the provided values

: undefined
}
/>
<SloEditForm onSave={onClose} initialValues={initialValues} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let the Form handles the merging of the provided values with default values

'xpack.slo.sloEdit.sliType.timesliceMetric.deleteLabel',
{ defaultMessage: 'Delete metric' }
)}
<EuiFlexGroup direction="column" gutterSize="xs">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🖌️ Mostly UI change


export const getDataViewPattern = ({
export const getDataViewPatternOrId = ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name was confusing me

Comment on lines +67 to +73
const updateDataViewDependantFields = (indexPattern?: string, timestampField?: string) => {
setValue(INDEX_FIELD, indexPattern ?? '');
setValue(INDICATOR_TIMESTAMP_FIELD, timestampField ?? '');
setValue(GROUP_BY_FIELD, ALL_VALUE);
setValue(SETTINGS_SYNC_FIELD, null);
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We make sure to reset the data views dependant fields to avoid keeping non-existant values

@kdelemme kdelemme marked this pull request as ready for review November 22, 2024 16:13
@kdelemme kdelemme requested a review from a team as a code owner November 22, 2024 16:13
@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-management Observability Management User Experience Team labels Nov 22, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@kdelemme kdelemme added release_note:feature Makes this part of the condensed release notes v8.17.0 labels Nov 22, 2024
Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 2, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
slo 855 857 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
observability 472.6KB 472.8KB +150.0B
slo 842.6KB 848.0KB +5.4KB
synthetics 1.1MB 1.1MB +97.0B
total +5.6KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
slo 4 2 -2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
slo 28.8KB 28.8KB +6.0B

History

cc @kdelemme

@kdelemme kdelemme merged commit 8fe4c44 into elastic:main Dec 2, 2024
9 checks passed
@kdelemme kdelemme deleted the feat/advanced-settings-slo branch December 2, 2024 21:19
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12128161463

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Backport failed because of merge conflicts

You might need to backport the following PRs to 8.x:
- [Security Solution] - Update codeowners (#202046)
- [eem] _search sort_by and display_name (#202361)
- [OpenAPI] Fix Serverless API base URL (#202373)
- [Synthetics] Fix to handle ops genie as default connector !! (#201923)
- [Security Solution] [Onboarding] t1_analyst role blocked from interacting with cards due to missing integration privileges (#202413)

Manual backport

To create the backport manually run:

node scripts/backport --pr 200822

Questions ?

Please refer to the Backport tool documentation

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 200822 locally

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 200822 locally

hop-dev pushed a commit to hop-dev/kibana that referenced this pull request Dec 5, 2024
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 200822 locally

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 200822 locally

CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 9, 2024
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 200822 locally

@kdelemme
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kdelemme added a commit to kdelemme/kibana that referenced this pull request Dec 10, 2024
…200822)

(cherry picked from commit 8fe4c44)

# Conflicts:
#	oas_docs/output/kibana.serverless.yaml
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

1 similar comment
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kdelemme added a commit that referenced this pull request Dec 13, 2024
…00822) (#203575)

# Backport

This will backport the following commits from `main` to `8.x`:
- [feat(slo): allow configuration of advanced settings from UI
(#200822)](#200822)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Kevin
Delemme","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-02T21:19:17Z","message":"feat(slo):
allow configuration of advanced settings from UI
(#200822)","sha":"8fe4c4419222f346bb6c953ca97422bd977e12f7","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["backport
missing","v9.0.0","release_note:feature","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-management","v8.18.0"],"number":200822,"url":"https://github.com/elastic/kibana/pull/200822","mergeCommit":{"message":"feat(slo):
allow configuration of advanced settings from UI
(#200822)","sha":"8fe4c4419222f346bb6c953ca97422bd977e12f7"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/200822","number":200822,"mergeCommit":{"message":"feat(slo):
allow configuration of advanced settings from UI
(#200822)","sha":"8fe4c4419222f346bb6c953ca97422bd977e12f7"}},{"branch":"8.x","label":"v8.18.0","labelRegex":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:project-deploy-observability Create an Observability project release_note:feature Makes this part of the condensed release notes Team:obs-ux-management Observability Management User Experience Team v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SLO] Settings configuration Home > SLOs -- Creating a new SLO, form fields do not indicate required fields.
4 participants