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 monitors configuration options #102928

Conversation

dominiqueclarke
Copy link
Contributor

@dominiqueclarke dominiqueclarke commented Jun 22, 2021

Summary

Relates to elastic/uptime#276

This PR adds the corresponding UI to support browser monitor configuration within the Synthetics integration package.

Screen Shot 2021-06-22 at 9 43 11 PM

Screen Shot 2021-06-22 at 9 43 04 PM

Testing
This feature involves coordinated changes from kibana, integrations, and beats. In order to test e2e, it is necessary to pull down each PR separately. However, there are a few things you can test without every piece.

  1. If you only have Kibana
    • Evaluate the design and flow
    • Evaluate that the monitor type dropdown only shows datastreams available from the integration package. To do this, you can change the version in the url to 0.2.1, which only contains http, tcp, and icmp and confirm that the monitor type dropdown only contains http, tcp, and icmp.
  2. If you have Kibana and the integration package
    • Save the integration policy and verify the keys in the integration policy yaml are correct and correspond to the appropriate heartbeat configuration fields. Reference: https://www.elastic.co/guide/en/beats/heartbeat/current/monitor-browser-options.html
    • Edit the integration policy, ensure all the inputs have the appropriate value, change fields, resave the policy, and verify the keys have been updated in the integration policy yaml
  3. If you have Kibana, the integration package, and agent
    • Test that browser monitors with configured with a zip url are processed by agent and appear in Uptime
    • Test that browser monitors configured with inline scripts are processed by agent and appear in Uptime

FULL DEV ENVIRONMENT SETUP INSTRUCTIONS: https://docs.google.com/document/d/1mS_94T0zRBtdJZVWLWc_B7iswTfkrGJXn9XMyt9JZjY/edit

Checklist

Uptime PR Checklist

The following considerations have been addressed

  • Accessibility has been considered, relevant aria tags and other accessibility features implemented
  • Telemetry has been added where relevant
  • Docs have been added to this PR covering any new, changed, or removed features
  • Testing for expected behavior is in place
    • Automated tests exist to cover expected and edge case conditions
    • User acceptance testing has been carried out to ensure the feature functions as expected within the context of how it will be used
    • Any special/edge cases that need to be manually tested must be documented
    • Ensure the new layout works responsively (including down to small phone widths, where makes sense for the user flow, e.g. the error page when reacting to an alert)

@dominiqueclarke dominiqueclarke added release_note:enhancement enhancement New value added to drive a business result v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.14.0 auto-backport Deprecated: Automatically backport this PR after it's merged labels Jun 22, 2021
@dominiqueclarke dominiqueclarke requested a review from a team as a code owner June 22, 2021 14:46
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@dominiqueclarke dominiqueclarke marked this pull request as draft June 22, 2021 14:46
: undefined
}
isInvalid={!!validate[ConfigKeys.RESPONSE_HEADERS_CHECK]?.(fields)}
error={[
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not remember why this was made an array. My gut tells me that this is not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably ok to change it then; it seems to accept ReactNode | ReactNode[], so we aren't losing anything by simplifying it to a string.

@dominiqueclarke dominiqueclarke marked this pull request as ready for review June 23, 2021 02:18
}
>
<SourceField
onChange={useCallback(
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm inline memo, or call back hooks, i was tempted to do this other day , not sure if it has any implications, probably none.

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as it's not conditional I think it's fine. My preference is to keep all hook usage isolated to its own block in the root of the function though, just because I find it to be more readable.

@shahzad31
Copy link
Contributor

Initial smoke testing seems to be fine
no validation for URL fields?
image

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.

Just some basic comments, code is looking pretty good.

}
>
<SourceField
onChange={useCallback(
Copy link
Contributor

Choose a reason for hiding this comment

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

As long as it's not conditional I think it's fine. My preference is to keep all hook usage isolated to its own block in the root of the function though, just because I find it to be more readable.

error={
<FormattedMessage
id="xpack.uptime.createPackagePolicy.stepConfigure.monitorIntegrationSettingsSection.timeout.error"
defaultMessage="Timeout must be 0 or greater and less than schedule interval"
Copy link
Contributor

Choose a reason for hiding this comment

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

This copy is confusing. Could we break this up and conditionally render one or the other?

// pseudocode
error={
  timeout < 0
    ? <FormattedMessage id="xpack.uptime.createPackagePolicy.stepConfigure.monitorIntegrationSettingsSection.timeout.lessThanZeroError"
      defaultMessage="Timeout may not be less than 0"
      />
    : <FormattedMessage id="greaterThanTimeout"
        defaultMessage=""Timeout cannot be less than schedule interval"
      />

{
id: 'syntheticsBrowserZipURLConfig',
name: (
<FormattedMessage
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems we are repeating this definition in the content section below, extract to a common definition above?

@dominiqueclarke
Copy link
Contributor Author

@elasticmachine merge upstream

@spalger spalger added v7.15.0 and removed v7.14.0 labels Jun 30, 2021
@dominiqueclarke
Copy link
Contributor Author

@elasticmachine merge upstream

@dominiqueclarke
Copy link
Contributor Author

@elasticmachine merge upstream

@dominiqueclarke dominiqueclarke force-pushed the feature/276-synthetics-integration-browser-monitors branch from f158d1c to 4919aba Compare August 13, 2021 19:54
@justinkambic
Copy link
Contributor

During smoke testing I did notice a weird little UI glitch where the form says the script is required, even when there's a valid script present:

image

@dominiqueclarke
Copy link
Contributor Author

@elasticmachine merge upstream


return (
!timeout ||
parseFloat(timeout as ICustomFields[ConfigKeys.TIMEOUT]) < 0 ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Could extract this cast to a var before the return statement, to avoid the repetition.

parseInt(fields[ConfigKeys.TIMEOUT], 10) < 0 ? (
<FormattedMessage
id="xpack.uptime.createPackagePolicy.stepConfigure.monitorIntegrationSettingsSection.timeout.lessThanZeroError"
defaultMessage="Timeout must be less than 0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this is a typo.

Suggested change
defaultMessage="Timeout must be less than 0"
defaultMessage="Timeout must be greater than 0"

) : (
<FormattedMessage
id="xpack.uptime.createPackagePolicy.stepConfigure.monitorIntegrationSettingsSection.timeout.moreThanIntervalError"
defaultMessage="Timeout cannot be more than the monitor interval"
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 "greater than" might be a little nicer. Just my opinion though.

Suggested change
defaultMessage="Timeout cannot be more than the monitor interval"
defaultMessage="Timeout cannot be greater than the monitor interval"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this to say Timeout must be less than the monitor interval. I thought that was more consistent. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine to me.

@@ -458,7 +519,7 @@ describe('<SyntheticsPolicyEditExtension />', () => {
const urlError = getByText('URL is required');
const monitorIntervalError = getByText('Monitor interval is required');
const maxRedirectsError = getByText('Max redirects must be 0 or greater');
const timeoutError = getByText('Timeout must be 0 or greater and less than schedule interval');
const timeoutError = getByText('Timeout must be less than 0');
Copy link
Contributor

Choose a reason for hiding this comment

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

Think this is a typo.

Suggested change
const timeoutError = getByText('Timeout must be less than 0');
const timeoutError = getByText('Timeout must be greater than 0');

await waitFor(() => {
const hostError = getByText('Zip URL is required');
const monitorIntervalError = getByText('Monitor interval is required');
const timeoutError = getByText('Timeout must be less than 0');
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
const timeoutError = getByText('Timeout must be less than 0');
const timeoutError = getByText('Timeout must be greater than 0');

helpText={
<FormattedMessage
id="xpack.uptime.createPackagePolicy.stepConfigure.monitorIntegrationSettingsSection.tags.helpText"
defaultMessage="A list of tags that will be sent with the monitor event. Press enter to add a new tab. Displayed in Uptime and enables searching by tag."
Copy link
Contributor

Choose a reason for hiding this comment

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

Again I'm not a copy expert but I think this conveys the necessary information. cc @andrewvc

Suggested change
defaultMessage="A list of tags that will be sent with the monitor event. Press enter to add a new tab. Displayed in Uptime and enables searching by tag."
defaultMessage="A list of tags to apply to the monitor event. Press enter to add a new tab. Enables searching by tag."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

),
content: (
<EuiFormRow
isInvalid={!!config.inlineScript}
Copy link
Contributor

Choose a reason for hiding this comment

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

You didn't implement a custom AST analyzer? 😉

[ConfigKeys.TIMEOUT]: (fields) => secondsToCronFormatter(fields[ConfigKeys.TIMEOUT]),
};

export const arrayToYamlFormatter = (value: string[] = []) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

This code doesn't seem like it's directly tested.

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's true. It is covered by functional tests that ultimately test the input of the form to the output of the yaml integration policy, but I'll add tests.

dominiqueclarke and others added 7 commits August 16, 2021 12:27
…simple_fields.tsx

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>
…source_field.tsx

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>
…ields.test.tsx

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>
…ple_fields.tsx

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>
@dominiqueclarke dominiqueclarke force-pushed the feature/276-synthetics-integration-browser-monitors branch from 89c4e41 to 8fca58f Compare August 16, 2021 19:17
@justinkambic
Copy link
Contributor

This is looking great, I've done another round of smoke testing and it appears to be working fantastically. You've addressed most of the necessary code as well. I have noticed that the false error message I noticed in my earlier review is still happening. Haven't noticed any other blocking issues though.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
uptime 566 588 +22

Async chunks

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

id before after diff
uptime 954.5KB 982.9KB +28.4KB

History

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

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.

LGTM!

@dominiqueclarke dominiqueclarke merged commit 8bfd5e2 into elastic:master Aug 17, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 17, 2021
…ptions (elastic#102928)

* update types

* add browser context

* update validation

* add browser fields to custom fields

* add browser simple fields and source field

* add browser context to create and edit wrappers

* update content

* add formatters for formatting javascript values to integration policy yaml

* fix policy name bug

* adjust tests

* adjust types

* add normalizers for converting yaml to javascript

* update default values

* add params field to browser monitors

* adjust types, formatters and normalizers to account for browser advanced fields

* add browser advanced fields context and components

* adjust http and tcp providers

* adjust use_update_policy and wrappers

* update types

* update content

* adjust timeout content

* adjust zip url content

* adjust content

* remove unused content

* hide monitor options that do not have corresponding data streams from the integration package

* Update x-pack/plugins/uptime/public/components/fleet_package/browser/simple_fields.tsx

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>

* Update x-pack/plugins/uptime/public/components/fleet_package/browser/source_field.tsx

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>

* Update x-pack/plugins/uptime/public/components/fleet_package/custom_fields.test.tsx

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>

* Update x-pack/plugins/uptime/public/components/fleet_package/http/simple_fields.tsx

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>

* adjust content

* adjust validation

* adjust tests

* adjust normalizers and formatters and add tests

* resolves validation error with inline scripts

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Justin Kambic <justin.kambic@elastic.co>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Aug 17, 2021
…ptions (#102928) (#108911)

* update types

* add browser context

* update validation

* add browser fields to custom fields

* add browser simple fields and source field

* add browser context to create and edit wrappers

* update content

* add formatters for formatting javascript values to integration policy yaml

* fix policy name bug

* adjust tests

* adjust types

* add normalizers for converting yaml to javascript

* update default values

* add params field to browser monitors

* adjust types, formatters and normalizers to account for browser advanced fields

* add browser advanced fields context and components

* adjust http and tcp providers

* adjust use_update_policy and wrappers

* update types

* update content

* adjust timeout content

* adjust zip url content

* adjust content

* remove unused content

* hide monitor options that do not have corresponding data streams from the integration package

* Update x-pack/plugins/uptime/public/components/fleet_package/browser/simple_fields.tsx

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>

* Update x-pack/plugins/uptime/public/components/fleet_package/browser/source_field.tsx

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>

* Update x-pack/plugins/uptime/public/components/fleet_package/custom_fields.test.tsx

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>

* Update x-pack/plugins/uptime/public/components/fleet_package/http/simple_fields.tsx

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>

* adjust content

* adjust validation

* adjust tests

* adjust normalizers and formatters and add tests

* resolves validation error with inline scripts

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Justin Kambic <justin.kambic@elastic.co>

Co-authored-by: Dominique Clarke <doclarke71@gmail.com>
Co-authored-by: Justin Kambic <justin.kambic@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated: Automatically backport this PR after it's merged enhancement New value added to drive a business result release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants