Slo overview embeddable instances api#245371
Conversation
github-actions
bot
added
the
author:actionable-obs
mgiota
force-pushed
the
slo-overview-embeddable-instances-api
branch
from
2d6e4fa to
56a93af
Compare
mgiota
force-pushed
the
slo-overview-embeddable-instances-api
branch
4 times, most recently
from
22c6e69 to
3aa1b07
Compare
mgiota
force-pushed
the
slo-overview-embeddable-instances-api
branch
from
02ef0a8 to
95eeda8
Compare
dmlemeshko
left a comment
dmlemeshko
left a comment
There was a problem hiding this comment.
x-pack/solutions/observability/test/api_integration_deployment_agnostic/services/slo_api.ts changes lgtm
mgiota
force-pushed
the
slo-overview-embeddable-instances-api
branch
from
7875898 to
7cd1e19
Compare
kdelemme
left a comment
kdelemme
left a comment
There was a problem hiding this comment.
Code lgtm, tested locally and works fine. Just a note on the tests, we can simplify them and make them run faster
| const [response1, response2] = await Promise.all([ | ||
| sloApi.create(slo1, adminRoleAuthc), | ||
| sloApi.create(slo2, adminRoleAuthc), | ||
| ]); |
There was a problem hiding this comment.
We don't need to create SLOs for this test, since we rely only on summary documents, we can just insert a bunch of summary documents with various names, etc... You can take a look at the x-pack/solutions/observability/test/api_integration_deployment_agnostic/apis/slo/purge_instances.ts tests.
Doing so we can also remove the need for the generate data part and the dataview, etc... The tests will be much faster to run.
...ck/solutions/observability/plugins/slo/public/hooks/use_fetch_slo_definitions_with_remote.ts
Show resolved
Hide resolved
x-pack/solutions/observability/plugins/slo/server/services/search_slo_definitions.test.ts
Show resolved
Hide resolved
| ) {} | ||
|
|
||
| public async execute(params: SearchSLODefinitionsParams): Promise<SearchSLODefinitionResponse> { | ||
| const { search, size = 10, searchAfter } = params ?? {}; |
There was a problem hiding this comment.
And one last thing while you update the integration test, let's add some checks on the size > 0 && size <= 100 for now
kdelemme
left a comment
kdelemme
left a comment
There was a problem hiding this comment.
Approving to unblock, but let's rewrite the integration tests without creating real SLOs (we can avoid them) and add some checks on the size param 👍🏻
|
@mgiota, it looks like you're updating the parameters for a rule type! Please review the guidelines for making additive changes to rule type parameters and determine if your changes require an intermediate release. |
⏳ Build in-progress, with failures
Failed CI StepsTest Failures
History
cc @mgiota |
@kdelemme Do you know what might have triggered this automated rule type parameter check? I haven't modified any existing rule type schemas in this PR. The comment seems to be a false positive. I am waiting for CI to report back, once it is green, I plan to merge the PR. |
This is a follow up PR to this [one](#245371) that improves tests and addresses: - [search_slo_definitions integration tests](x-pack/solutions/observability/test/api_integration_deployment_agnostic/apis/slo/search_slo_definitions.ts): no need to create SLOs for the test, since we rely only on summary documents. We can just insert a bunch of summary documents with various names, etc... - [search_slo_definitions unit tests](x-pack/solutions/observability/plugins/slo/server/services/search_slo_definitions.ts): add some checks on the size > 0 && size <= 100 cc @kdelemme --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Fixes #243996
Screen.Recording.2025-12-05.at.14.16.07.mov
Remote SLOs
Screen.Recording.2026-01-16.at.15.25.54.mov