Skip to content

[Synthetics Service] Add warnings for when service nodes bandwidth is exceeded#127961

Merged
lucasfcosta merged 3 commits intoelastic:mainfrom
lucasfcosta:warning-for-bandwidth-exceeded-on-service
Mar 24, 2022
Merged

[Synthetics Service] Add warnings for when service nodes bandwidth is exceeded#127961
lucasfcosta merged 3 commits intoelastic:mainfrom
lucasfcosta:warning-for-bandwidth-exceeded-on-service

Conversation

@lucasfcosta
Copy link
Copy Markdown
Contributor

@lucasfcosta lucasfcosta commented Mar 17, 2022

⚠️ WARNING: Please notice that part of this functionality (latency alerts) should not be shown anymore as per the following PR: #129597. Make sure to test the requirements in that PR instead of this one.

Summary

This PR addresses the Synthetics Service issue about network bandwidth limits. I can't link it here as it's on a non-public repo.

It does so by adding warnings for when the user exceeds throttling limits, or does not enable throttling when assigning monitors to a location.

Screenshot 2022-03-17 at 11 24 38

Screenshot 2022-03-17 at 11 24 27

How to test this PR Locally

  1. Create a new manifest.json with the following content and place it under a folder named, for example, manifest_folder.
    {
      "throttling": {
        "downloadBandwidthLimit": 20,
        "uploadBandwidthLimit": 10,
        "latencyLimit": 5000
      },
      "locations": {
        "Local Synth Node [MANIFEST]": {
          "url": "https://localhost:10001",
          "geo": {
            "name": "Example - Local",
            "location": {"lat": 41.25, "lon": -95.86}
          },
          "status": "beta"
        }
      }
    }
  2. Run an HTTP server to serve the contents of the manifest_folder
    $ npx http-server manifest_folder --port 8082
    
  3. In Kibana's config, fill the following options:
    # Make sure your pods will be able to send data to ES which should be bound to your host's 9200 port
    xpack.uptime.service.hosts: [http://host.docker.internal:9200]
    
    # Add the URL for the manifest with the `throttling` configs you're serving
    xpack.uptime.service.manifestUrl: http://localhost:8082/manifest.json
    
    # Make sure to disable the devUrl so you're only fetching locations from the `manifest` as we'll do in production
    # xpack.uptime.service.devUrl: https://localhost:10001
    
    # Enable the Monitor Management UI
    xpack.uptime.ui.monitorManagement.enabled: true
  4. Run the service locally
    $ DEV=true LOGLEVEL=trace CACERTFILE=./k8s/cert-test/build/bundle.pem SERVERCERT=./k8s/cert-test/build/local-server.pem SERVERKEYFILE=./k8s/cert-test/build/local-server.key PORT=10001 ./synthetics-service
    
  5. Run Kibana and try to add a monitor. You should see the warnings above when exceeding the limits defined in your manifest.json file.
  6. Disable throttling and you should see a warning about the automatic caps.
  7. Create a monitor with a particular throttling value set and save it. Use the following code for that monitor:
    step("load homepage", async () => {
        await page.goto('https://www.fast.com');
    });
    
    step("wait for test", async () => {
      await page.waitForSelector('#show-more-details-link');
      await page.waitForTimeout(10000);
      await page.click("#show-more-details-link");
    });
    
    step("wait for up/lat", async () => {
      await page.waitForSelector(".extra-measurement-value-container.succeeded");
      await page.waitForTimeout(10000);
    });
  8. Wait for that monitor to run and ensure your throttling values have been applied.
  9. Create a monitor with throttling disabled using the same script as above and confirm that your script has not been throtted.
    ⚠️ It will be throttled on the service because of platform limits.
  10. Create a monitor with throttling values which exceed the limits for the node. You should still be able to save such monitors.

Checklist

Delete any items that are not applicable to this PR.

  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Documentation was added for features that require explanation or tutorials -> Do we need this now or should we wait until we're out of beta?
  • Unit or functional tests were updated or added to match the most common scenarios
  • Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
  • Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
  • This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
  • This was checked for cross-browser compatibility

For maintainers

Release note

Show warning when users exceed a Synthetics Node throttling limits.

@lucasfcosta lucasfcosta added release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.2.0 labels Mar 17, 2022
@lucasfcosta lucasfcosta requested a review from a team as a code owner March 17, 2022 11:38
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/uptime (Team:uptime)

@paulb-elastic paulb-elastic requested a review from liciavale March 17, 2022 12:01
@paulb-elastic
Copy link
Copy Markdown
Contributor

Added @liciavale / @drewpost as specific reviewers

@paulb-elastic paulb-elastic requested a review from drewpost March 17, 2022 12:02
@paulb-elastic
Copy link
Copy Markdown
Contributor

I understand the max values are the limit, so the copy should probably read something like less than or equal to, or can’t be larger than.

When exceeded, maybe the copy should include something about limit and then use a similar wording to when throttling is disabled, for example When using throttling values larger than a Synthetics Node bandwidth limit, your monitor will still have its bandwidth capped...?

Also, I think we’re letting users continue to save the configuration anyway, I assume we have no distinction in the UI, and this shows as a validation failure?

@lucasfcosta lucasfcosta force-pushed the warning-for-bandwidth-exceeded-on-service branch from 16dd7e9 to dc76a75 Compare March 17, 2022 12:20
@lucasfcosta
Copy link
Copy Markdown
Contributor Author

Hey Paul, thanks for jumping in on this. Wrt to your comments:

I understand the max values are the limit, so the copy should probably read something like less than or equal to, or can’t be larger than.

That's an excellent point, will update.

When exceeded, maybe the copy should include something about limit and then use a similar wording to when throttling is disabled, for example When using throttling values larger than a Synthetics Node bandwidth limit, your monitor will still have its bandwidth capped...?

I really like your suggestion of: When using throttling values larger than a Synthetics Node bandwidth limit, your monitor will still have its bandwidth capped. Much more direct and less wordy too.

Also, I think we’re letting users continue to save the configuration anyway, I assume we have no distinction in the UI, and this shows as a validation failure?

That is correct. We have no distinction in the UI. I could only use the validation error message or a callout or another element. There isn't a specific warning prop on the form elements for this. We do still allow users to save when they see these warning messages.

@lucasfcosta lucasfcosta force-pushed the warning-for-bandwidth-exceeded-on-service branch 2 times, most recently from 5f20a56 to 7a751a5 Compare March 17, 2022 15:56
@lucasfcosta
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@lucasfcosta lucasfcosta force-pushed the warning-for-bandwidth-exceeded-on-service branch from ddfe0f1 to d5ba2e7 Compare March 18, 2022 10:21
@lucasfcosta
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@lucasfcosta lucasfcosta requested a review from hbharding March 22, 2022 19:33
Comment on lines 46 to 48
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i wonder if this can be set to a constant, seems like these three lines are repeated in many places

something like DEFAULT_THROTTLING

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

basically reuse this in bunch of places...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

basically reuse this in bunch of places...

@dominiqueclarke dominiqueclarke self-requested a review March 24, 2022 00:48
Copy link
Copy Markdown
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

Looks great! Just one comment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It may make sense to determine whether or not this is the service flow in some other way. With this logic, the warnings will only appear if location has already been set. While it is likely that the user will choose a location first, I think these warnings should still appear even when they do not.

We could add a runsOnService boolean directly to the PolicyConfigContext that is only defined within the UIMonitorManagement workflow.

Copy link
Copy Markdown
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.

i think this callout is too prominent , can this perhaps be a an icon tooltip?
Point being this almost feels like user is doing something wrong.

Instead of saying Disable throttling, we should change this switch to say

Enable default throttling

then we will not need this callout

image

@drewpost
Copy link
Copy Markdown

This looks good for beta. We can wordsmith it and tweak it a bit if needed in synthetics app but I'd say this is good to go for now!

@lucasfcosta lucasfcosta force-pushed the warning-for-bandwidth-exceeded-on-service branch 2 times, most recently from 9cb9b0d to b8edcc5 Compare March 24, 2022 13:19
@lucasfcosta
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@lucasfcosta lucasfcosta force-pushed the warning-for-bandwidth-exceeded-on-service branch from 43041b1 to 5d29c42 Compare March 24, 2022 17:21
Copy link
Copy Markdown
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 !!

@lucasfcosta lucasfcosta force-pushed the warning-for-bandwidth-exceeded-on-service branch from 5d29c42 to 9c1eecb Compare March 24, 2022 18:29
@kibana-ci
Copy link
Copy Markdown

💚 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 755.8KB 758.8KB +3.0KB

History

  • 💚 Build #33552 succeeded 43041b105df9606b476912365cd25b065961714c
  • 💔 Build #33480 failed b8edcc536fbc18e87e016c38b8d8f9da043312a3
  • 💔 Build #33409 failed 9cb9b0dd8875ec700c8f676ac363d0f08728fbdd
  • 💚 Build #33134 succeeded 5e4dc1c56f1a9c21e323733606cd97c63523984d
  • 💚 Build #32081 succeeded 57b2d55984d803762485876e51c6e94bb66a30bf
  • 💚 Build #31742 succeeded d5ba2e7cad7c6d5c2dfa0ad9c8d3f3ebcd05c1a9

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

@lucasfcosta lucasfcosta merged commit c9af8f0 into elastic:main Mar 24, 2022
@lucasfcosta lucasfcosta deleted the warning-for-bandwidth-exceeded-on-service branch March 24, 2022 20:04
@kibanamachine
Copy link
Copy Markdown
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 127961 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 28, 2022
@kibanamachine
Copy link
Copy Markdown
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 127961 or prevent reminders by adding the backport:skip label.

@spalger spalger added the backport:skip This PR does not require backporting label Mar 30, 2022
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants