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

Open Synthetics recorder directly from kibana via link #77

Merged
merged 1 commit into from
Feb 11, 2022

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Nov 17, 2021

⚠️ As requested by @shahzad31, I (@lucasfcosta) am now responsible for this PR, so I'll be the ones pushing commits and responding to comments in this issue.

Summary

This PR adds the ability to open Recorder directly from Kibana (or any other website) through a link using the elastic-synthetics-recorder-protocol.

You could, for example, use the following EuiLink to open the recorder:

<EuiLink href={`elastic-synthetics-recorder://`}>Open Script Recorder</EuiLink>

Please notice that this PR/task does not include adding the relevant links in Kibana, and that that will be covered separately in elastic/kibana#120638.

For now, there is no way to determine that the recorder is installed or not. Therefore, users which do not have the recorder installed will see the following standard error:

Standard error when the script is not installed

Below is a gif of the link working:

2021-11-17 12-15-22 2021-11-17 12_17_13

How to test this PR

  1. Clone this branch and package the application using npm run package
  2. Once the application is packaged, mount its dmg and install the .app into Applications.
  3. Create a new folder example-folder and add the following HTML file as index.html to it:
    <!DOCTYPE html>
    <html lang="en">
    <head>
      <meta charset="UTF-8">
      <title>Test Recorder Link</title>
    </head>
    <body>
      <a href="elastic-synthetics-recorder://">Open recorder</a>
    </body>
    </html>
    
  4. Serve the HTML file in that folder using npx http-server example-folder
  5. Access the localhost URL in which you're serving your file and click the Open recorder link. The script recorder should open.

@shahzad31 shahzad31 marked this pull request as ready for review November 17, 2021 11:23
@shahzad31 shahzad31 added the enhancement New feature or request label Nov 17, 2021
@shahzad31 shahzad31 changed the title added protocol Open Synthetics recorder directly from kibana via link Nov 17, 2021
@justinkambic
Copy link
Contributor

I like this, curious to hear what @drewpost and @liciavale would think about it, as we haven't really discussed/refined for this functionality AFAIK.

The happy path seems really nice. When we say "it will throw an error" if the protocol isn't present, what does that end up looking like on the Kibana side? Something we can catch and toast about?

package.json Outdated
@@ -48,7 +48,14 @@
"main": "build/electron.js"
},
"mac": {
"icon": "public/elastic.png"
"icon": "public/elastic.png",
"category": "elastic.synthetics.recorder"
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this, Synthetics should be part of devtools category (https://developer.apple.com/library/archive/documentation/General/Reference/InfoPlistKeyReference/Articles/LaunchServicesKeys.html#//apple_ref/doc/uid/TP40009250-SW8) and we will have to do this while notarizing the app #51

package.json Outdated
"protocols": {
"name": "ElasticSyntheticsRecorder-protocol",
"schemes": [
"ElasticSyntheticsRecorder"
Copy link
Member

@vigneshshanmugam vigneshshanmugam Nov 18, 2021

Choose a reason for hiding this comment

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

please keep it URL spec compatible like keeping it lower case.

@paulb-elastic
Copy link
Contributor

I agree with @justinkambic's comment, how does it look to the user if they don't have the script recorder installed?

@shahzad31
Copy link
Contributor Author

@paulb-elastic @justinkambic so that is UI problem, i mean in kibana, it won't break anything, if you click the link , it will just show standard error
image

there are many ways we can solve this on the UI, easiest would be to show a dismissable information first time, asking user to download the recorder , we can use EUI tour component to guide the user

image

Alternatively it's just a standard link, we can capture the error, and show our own dialog with info to download the recorder first.

Essentially, that has nothing to do with this PR, there isn't much we can do here.

@apmmachine
Copy link
Contributor

apmmachine commented Nov 29, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-12-07T14:55:22.120+0000

  • Duration: 8 min 50 sec

  • Commit: df8a462

Test stats 🧪

Test Results
Failed 0
Passed 30
Skipped 0
Total 30

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@lucasfcosta lucasfcosta self-assigned this Dec 6, 2021
@lucasfcosta
Copy link
Contributor

As discussed with @shahzad31 on Slack, I'll be picking this issue up as he's busy at the moment and I'll be working on the recorder more often.

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've looked into it a bit further and haven't found an OS-specific reason we can't merge this, so LGTM.

@lucasfcosta
Copy link
Contributor

As mentioned on slack by @justinkambic we'll wait until we have heard back from the security team on whether this can impact notarization:

One thing I’m wondering about this, is will it impact our desire to notarize the app via Elastic’s Apple Dev account. I haven’t fully investigated this yet, it seems innocuous and a really minimal change, but it might be good to verify before we merge this.

@shahzad31
Copy link
Contributor Author

As mentioned on slack by @justinkambic we'll wait until we have heard back from the security team on whether this can impact notarization:

One thing I’m wondering about this, is will it impact our desire to notarize the app via Elastic’s Apple Dev account. I haven’t fully investigated this yet, it seems innocuous and a really minimal change, but it might be good to verify before we merge this.

I can't possibly think why it will impact notarisation? Did you hear back @lucasfcosta , let's push this otherwise, then we can see how we want to use it in Kibana.

@lucasfcosta
Copy link
Contributor

lucasfcosta commented Jan 4, 2022

@shahzad31 I've pinged the security team about this subject this morning, and will open a consulting request on their repo as I was instructed. I'll keep this PR up-to-date with their responses and with the issue once it's there.

EDIT: Issue opened on their repo for consultancy (private repo). The link will appear below for those with permissions.

@lucasfcosta
Copy link
Contributor

Merging as per the security team's approval. Let's make sure we do another security review before passing data from Kibana to the recorder through the link.

@lucasfcosta lucasfcosta merged commit 73c9f1a into main Feb 11, 2022
@lucasfcosta lucasfcosta deleted the open-app-from-kibana branch February 11, 2022 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants