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] Show experimental locations only when a particular flag is enabled #132063

Merged
merged 2 commits into from
May 24, 2022

Conversation

lucasfcosta
Copy link
Contributor

@lucasfcosta lucasfcosta commented May 11, 2022

As a note for people doing post-FF testing in the future: please ensure you test this on different cloud regions so that we ensure staging and QA do show experimental locations but production doesn't.
For that, you can simply serve a manifest.json locally and point the cloud Kibana to it by editing Kibana settings (you'll have to expose it with something like ngrok).

Summary

This PR closes #131585. It allows the UI to hide experimental locations unless the xpack.uptime.service.showExperimentalLocations flag is set to true.

⚠️ Please notice that in order for us to show all locations by default on staging we should set this configuration flag to true on clusters provisioned there.

⚠️ The cloud team will have to update the manifest to set existing locations to GA once this gets merged.

Screenshot 2022-05-11 at 15 35 57

How to test this PR

You must test this PR using a Kibana distributable and running from source so that you can test the NODE_ENV as a developer and as if you were in production, respectively.

Testing in dev mode

  1. Create a manifest.json file with the following contents and place it under a manifests folder:
    {
      "throttling": {
        "download": 20,
        "upload": 10
      },
      "locations": {
        "Local Synth Node (GA) [MANIFEST]": {
          "url": "https://localhost:10001",
          "geo": {
            "name": "US East [GA]",
            "location": {"lat": 41.25, "lon": -95.86}
          },
          "status": "ga"
        },
        "Local Synth Node (Experimental) [MANIFEST]": {
          "url": "https://localhost:10001",
          "geo": {
            "name": "US West [EXP]",
            "location": {"lat": 41.25, "lon": -95.86}
          },
          "status": "experimental"
        }
      }
    }
  2. Use the http-server package to serve your manifest file.
    $ npx http-server service_manifest --port 8081
    
  3. Update your Kibana configs so that it fetches the manifest from your local server.
    # Make sure to comment the `devUrl` key
    # xpack.uptime.service.devUrl: https://localhost:10001
    xpack.uptime.service.manifestUrl: http://localhost:8081/manifest.json
    xpack.uptime.service.showExperimentalLocations: false
  4. Try to create a new monitor and you should see that all locations appear regardless of showExperimentalLocations being false.

Testing as if it was in production

⚠️ This is the most important test to do. It requires you to build a Kibana distributable so that you set process.env.NODE_ENV to production, and thus can properly test Kibana.
For that, you will have to:

  1. Build dist bundles
  2. Run the non-dev version of kibana NODE_ENV=production node --nolazy --inspect scripts/kibana.
  1. Create a manifest.json file with the following contents and place it under a manifests folder:
    {
      "throttling": {
        "download": 20,
        "upload": 10
      },
      "locations": {
        "Local Synth Node (GA) [MANIFEST]": {
          "url": "https://localhost:10001",
          "geo": {
            "name": "US East [GA]",
            "location": {"lat": 41.25, "lon": -95.86}
          },
          "status": "ga"
        },
        "Local Synth Node (Experimental) [MANIFEST]": {
          "url": "https://localhost:10001",
          "geo": {
            "name": "US West [EXP]",
            "location": {"lat": 41.25, "lon": -95.86}
          },
          "status": "experimental"
        }
      }
    }
  2. Use the http-server package to serve your manifest file.
    $ npx http-server service_manifest --port 8081
    
  3. Update your Kibana configs so that it fetches the manifest from your local server.
    # Make sure to comment the `devUrl` key
    # xpack.uptime.service.devUrl: https://localhost:10001
    xpack.uptime.service.manifestUrl: http://localhost:8081/manifest.json
    xpack.uptime.service.showExperimentalLocations: false
  4. Try to create a new monitor and you should see that only the US East [GA] location appears.
  5. Update the xpack.uptime.service.showExperimentalLocations key to true and ensure that both locations appear and that the experimental location has a Tech Preview badge.

⚠️ The same should happen when editing a monitor.

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

    ⚠️ I don't believe this is necessary, but I'd appreciate it if reviewers could confirm.

  • 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

Kibana will only show experimental Synthetic node locations when the xpack.uptime.service.showExperimentalLocations flag is set to true.

@lucasfcosta lucasfcosta added release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.3.0 labels May 11, 2022
@lucasfcosta lucasfcosta marked this pull request as ready for review May 17, 2022 09:10
@lucasfcosta lucasfcosta requested a review from a team as a code owner May 17, 2022 09:10
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@lucasfcosta
Copy link
Contributor Author

Considering this PR defaults to not showing experimental locations I think it should be fine to merge this one while we wait for cloud to set the defaults on Staging and QA to true 😄

@lucasfcosta lucasfcosta force-pushed the show-ga-locations-only branch 2 times, most recently from d19720b to b2d4e05 Compare May 19, 2022 09:19
@@ -41,13 +43,22 @@ export async function getServiceLocations(server: UptimeServerSetup) {
locations: Record<string, ManifestLocation>;
}>(server.config.service!.manifestUrl!);

Object.entries(data.locations).forEach(([locationId, location]) => {
const isProd = process.env.NODE_ENV === 'production';
Copy link
Contributor

Choose a reason for hiding this comment

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

i will recommend using flag that is coming from plugin context to determine if the evn is dev or prod

Copy link
Contributor

Choose a reason for hiding this comment

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

@kibana-ci
Copy link
Collaborator

💚 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
synthetics 792.7KB 793.1KB +390.0B

History

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

@lucasfcosta lucasfcosta enabled auto-merge (squash) May 23, 2022 16:15
@lucasfcosta
Copy link
Contributor Author

@shahzad31 this is ready for a re-review. I've also enabled auto-merge given I've already tested it myself and addressed your changes, so it can go in as soon as you approve, thanks a lot 🙌

@dominiqueclarke dominiqueclarke self-requested a review May 23, 2022 19:07
Copy link
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.

LGTM

@lucasfcosta lucasfcosta merged commit 83a05ff into elastic:main May 24, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 24, 2022
@mistic
Copy link
Member

mistic commented May 24, 2022

@lucasfcosta this PR has made our CI to fail on main. I've opened a PR at #132764 which I think it will make the CI green again

@lucasfcosta
Copy link
Contributor Author

@mistic ah, that's interesting, thanks for that. I wonder whether we should look into the PR checks in that case? I'd have expected the checks to catch that missing type 🤔

@mistic
Copy link
Member

mistic commented May 24, 2022

@lucasfcosta they should catch it, thats correct. In that case I think the problem was that the PR was not completely in sync with the upstream before the merge 😃

@lucasfcosta lucasfcosta deleted the show-ga-locations-only branch May 24, 2022 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Uptime] Handle "status" field in service manifest
7 participants