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

[Uptime][Synthetics Integration] add browser callout #108935

Conversation

dominiqueclarke
Copy link
Contributor

Summary

Fixes #108710

Adds an EuiCallout when the browser option is selected.

The current content is under review. The content was pulled directly from the issue, but will be update upon product and design review.

Screen Shot 2021-08-17 at 11 21 39 AM

@dominiqueclarke dominiqueclarke added enhancement New value added to drive a business result v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v7.15.0 labels Aug 17, 2021
@dominiqueclarke dominiqueclarke requested a review from a team as a code owner August 17, 2021 15:29
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

}
values={{
link: (
<EuiLink target="_blank" href="" external>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This link still needs to be provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

For now I'd say https://www.elastic.co/guide/en/observability/current/synthetic-monitoring.html is fine. I've opened elastic/observability-docs#989 to track adding more docs

@dominiqueclarke dominiqueclarke added bug Fixes for quality problems that affect the customer experience and removed enhancement New value added to drive a business result labels Aug 18, 2021
@mistic mistic added v7.16.0 and removed v7.15.0 labels Aug 18, 2021
@dominiqueclarke
Copy link
Contributor Author

@elasticmachine merge upstream

@dominiqueclarke
Copy link
Contributor Author

@elasticmachine merge upstream

@drewpost
Copy link

To create a "Browser" monitor, please ensure you are using the "complete" elastic-agent Docker container as this contains the dependencies to run these monitors which can be downloaded from HERE. For more information, please visit our synthetics documentation.

"here" links directly to the complete agent download instructions
"synthetic documentation" links to our docs

cc @dominiqueclarke

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

Had a question about copy, otherwise looking good.

title={
<FormattedMessage
id="xpack.uptime.createPackagePolicy.stepConfigure.monitorIntegrationSettingsSection.monitorType.browser.warning.description"
defaultMessage='Browser monitoring requires using the "complete" variant of the elastic–agent docker container. Other distributions of elastic that agent will not work correctly with it because they lack the required browser and other binaries. For more information see our {link}'
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='Browser monitoring requires using the "complete" variant of the elastic–agent docker container. Other distributions of elastic that agent will not work correctly with it because they lack the required browser and other binaries. For more information see our {link}'
defaultMessage='Browser monitoring requires using the "complete" variant of the elastic–agent docker container. Other distributions of elastic that agent will not work correctly with it because they lack the required browser and other binaries. For more information see our {link}.'

Also, this sentence:

Other distributions of elastic that agent will not work correctly with it because they lack the required browser and other binaries. 

This reads weirdly to me. Are we saying that "distributions of elastic" might not work correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This copy was a placeholder. Drew just provided the final version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops, sorry, I saw that he commented and was confused why they didn't match. Should've put 2 + 2 together!

@justinkambic
Copy link
Contributor

@elasticmachine merge upstream

@@ -122,6 +124,34 @@ export const CustomFields = memo<Props>(
/>
</EuiFormRow>
)}
<EuiSpacer size="s" />
{isBrowser && (
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to show this everytime user is adding a browser moniotor, is there a way to detect if they already have a working browser monitor, in that case i think user is probaly already educated and doesn;t need this additional context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. There isn't a way to do this easily. We'd have to make a call to a fleet api to collect the users policies and check from there. Alternatively, we could do a query against the datastream to see if it has any documents with monitor.type of browser.

It may be worth it to keep the call out though, since the user still need to make sure they are assigning the integration policy to the correct agent policy and they make have to change the selection at the bottom. Could be a good reminder.

@dominiqueclarke
Copy link
Contributor Author

@elasticmachine merge upstream

@shahzad31
Copy link
Contributor

@elasticmachine merge upstream

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 !!

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

I'm not going to hold this one up but I think we might want to highlight the container's name as code or something in the future. It can be a little hard to track the flow of the sentence without denoting that it's a container name.

image

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
uptime 1.0MB 1.0MB +1.3KB

History

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

@shahzad31 shahzad31 merged commit 4b33115 into elastic:master Sep 14, 2021
@shahzad31 shahzad31 deleted the feature/108710-synthetics-integration-add-browser-callout branch September 14, 2021 15:42
@kibanamachine
Copy link
Contributor

The following labels were identified as gaps in your version labels and will be added automatically:

  • v7.16.0

If any of these should not be on your pull request, please manually remove them.

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Sep 14, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Sep 14, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.15
7.x

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 14, 2021
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: shahzad31 <[email protected]>

Co-authored-by: Dominique Clarke <[email protected]>
Co-authored-by: shahzad31 <[email protected]>
kibanamachine added a commit that referenced this pull request Sep 14, 2021
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: shahzad31 <[email protected]>

Co-authored-by: Dominique Clarke <[email protected]>
Co-authored-by: shahzad31 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.15.0 v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Uptime] Add Info EuiCallOut for browser re: complete image
8 participants