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

Add SLO create callout to service overview, transactions page and transactions details #159958

Merged
merged 7 commits into from Jun 20, 2023

Conversation

MiriamAparicio
Copy link
Contributor

@MiriamAparicio MiriamAparicio commented Jun 19, 2023

We split the work for #157484 into this PR and another one that will add the links to create SLOs from the Alerts and Rules dropdown in APM. Also, the design for the callout was simplified and we will iterate again in 8.10

What was done

Added callout with links to create SLOs in service overview, transactions overview and transaction details.

Screen.Recording.2023-06-20.at.09.44.40.mov

@apmmachine
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 elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@MiriamAparicio MiriamAparicio changed the title Add call out to service overview, transactions page and transactions details Add SLO create callout to service overview, transactions page and transactions details Jun 19, 2023
@MiriamAparicio MiriamAparicio added Team:APM All issues that need APM UI Team support release_note:feature Makes this part of the condensed release notes apm:release-feature APM UI - Release Feature Goal v8.9.0 labels Jun 20, 2023
@MiriamAparicio MiriamAparicio marked this pull request as ready for review June 20, 2023 08:51
@MiriamAparicio MiriamAparicio requested a review from a team as a code owner June 20, 2023 08:51
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:APM)

@@ -76,13 +79,29 @@ export function ServiceOverview() {
? 'column'
: 'row';

const [sloCalloutDismissed, setSloCalloutDismissed] = useLocalStorage(
Copy link
Contributor

@yngrdyn yngrdyn Jun 20, 2023

Choose a reason for hiding this comment

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

Are we cleaning this value? if not, this user will not see the callout again unless he is using another computer, incognito mode, another window, etc. Is that the intention? if that's not the intention maybe using sessionStorage could help us here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, as far as I know if the user clicks to hide the callout it's not going to show again as we do in other places

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we discussed that the callout will be hidden if the user chooses to hide it or clicks the button to crate an SLO.

return (
<AnnotationsContextProvider
serviceName={serviceName}
environment={environment}
start={start}
end={end}
>
{!sloCalloutDismissed && (
Copy link
Contributor

@yngrdyn yngrdyn Jun 20, 2023

Choose a reason for hiding this comment

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

Shouldn't we also show this callout if an SLO is not current configured for this variables? or are we planning to show it even when the user has one configured?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I think after creating one we shouldn’t show the callout again, @gbamparop do we know if we can check if there are SLOs already created for the user?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point, we could reach to the AO team for this, but I would leave it simple for 8.9.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please create a follow up story for this

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 are dismissing the callout after navigating to create SLO, change added here 2a6aa66

Copy link
Contributor

@yngrdyn yngrdyn Jun 20, 2023

Choose a reason for hiding this comment

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

What about detecting if an SLO rule is already created? Maybe a follow up for that one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yngrdyn I created this issue #160017

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't offer yet an easy way to search for an SLO with specific params. You could search for all SLOs created with the sli.apm.transactionErrorRate or sli.apm.transactionDuration indicator, but that would require to handle pagination, etc.. I would not recommend doing this.

On a side note, are we planning of supporting creating SLO with the sli.apm.transactionDuration indicator with the threshold value set to a default value like the 95th (99th? avg?) percentile latency of the (service, env, transactionType, transactionName) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @kdelemme, I think for the porpoise of this, checking if there's at least one created will work for us.

<p>
<FormattedMessage
id="xpack.apm.slo.callout.description"
defaultMessage="How quickly will you respon if the service goes down? Keep the performance, speed and user experience high with a SLO"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
defaultMessage="How quickly will you respon if the service goes down? Keep the performance, speed and user experience high with a SLO"
defaultMessage="How quickly will you respond if the service goes down? Keep the performance, speed and user experience high with a SLO"

Copy link
Contributor

@yngrdyn yngrdyn left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +79 to +82
data-test-subj="apmCreateSloLink"
href={basePath.prepend(
`/app/observability/slos/create${sloParams}`
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

😍 Looking forward to see this in APM 👏🏻

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1406 1407 +1

Async chunks

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

id before after diff
apm 3.6MB 3.6MB +1.9KB
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 13 15 +2
securitySolution 411 415 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 14 16 +2
securitySolution 494 498 +4
total +6

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@MiriamAparicio MiriamAparicio merged commit ea0935f into elastic:main Jun 20, 2023
21 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 20, 2023
@MiriamAparicio MiriamAparicio deleted the 157484-integrate-SLO-apm branch June 20, 2023 14:18
@sorenlouv
Copy link
Member

@MiriamAparicio @kdelemme I just noticed from your recording that it is possible to create an SLO when "All" environments are selected. There is no environment called environment_all. It's a magic string we use in APM app that we check against. Unless the SLO app does the same it won't work (it's essentially creating an SLO for an environment that doesn't exist).

When "All environments" is selected in APM UI app I suggest passing undefined to the SLO app so no environment is selected.

@kdelemme
Copy link
Contributor

You can provide * to the params.environment field, and internally we'll handle it as no environment is selected:

  if (indicator.params.environment !== ALL_VALUE) {
      queryFilter.push({
        match: {
          'service.environment': indicator.params.environment,
        },
      });
    }

MiriamAparicio added a commit that referenced this pull request Jun 21, 2023
…160103)

Bug fix for
#159958 (comment)

### What was done

Provide * to the params.environment field when environment_all,
actionable observability will handle it internally
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:release-feature APM UI - Release Feature Goal backport:skip This commit does not require backporting release_note:feature Makes this part of the condensed release notes Team:APM All issues that need APM UI Team support v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants