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

Remove waitForSelector for some dashboards page #10

Merged
merged 7 commits into from
Jan 21, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions USER_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ You can also find this information using help command.
opensearch-reporting-cli --help
```

NOTE: The tenant in the url has the higher priority than tenant value provided as command option. For example, if the command is `opensearch-reporting-cli -u http://localhost:5601/goto/069af6d6f3294421ec163b07fef91e5d?security_tenant=private -t global` then tenant value *private* will be used for generating report because url contains *security_tenant=private*.

## Environment Variable File

Reporting CLI also reads environment variables from .env file in the current directory.
Expand Down
Binary file removed docs/dev/reporting_anything_flow_diagram.png
Binary file not shown.
3 changes: 3 additions & 0 deletions src/arguments.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ export async function getCommandArguments() {
.default(DEFAULT_EMAIL_SUBJECT)
.env(ENV_VAR.EMAIL_SUBJECT))

program.addHelpText('after', `
Note: The tenant in the url has the higher priority than tenant value provided as command option.`);

program.parse(process.argv);
const options = program.opts();
spinner.start('Fetching the arguments values');
Expand Down
22 changes: 0 additions & 22 deletions src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,6 @@ export const DEFAULT_MIN_HEIGHT = '600';
export const DEFAULT_FILENAME = 'opensearch-report';
export const DEFAULT_EMAIL_SUBJECT = 'This is an email containing your opensearch dashboard report';

export const REPORT_TYPE = {
DASHBOARD: 'Dashboard',
VISUALIZATION: 'Visualization',
NOTEBOOK: 'Notebook',
DISCOVER: 'Saved search',
OTHER: 'Other',
}

export const SELECTOR = {
DASHBOARD: '#dashboardViewport',
VISUALIZATION: '.visEditor__content',
NOTEBOOK: '.euiPageBody',
DISCOVER: 'button[id="downloadReport"]'
}

export const FORMAT = {
PDF: 'pdf',
PNG: 'png',
Expand All @@ -40,13 +25,6 @@ export const AUTH = {
NONE: 'none',
}

export const URL_SOURCE = {
DASHBOARDS: 'dashboards#',
VISUALIZE: "Visualize",
DISCOVER: "discover#",
NOTEBOOKS: "notebooks",
}

export const ENV_VAR = {
URL: 'OPENSEARCH_URL',
USERNAME: 'OPENSEARCH_USERNAME',
Expand Down
75 changes: 2 additions & 73 deletions src/download-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import puppeteer from 'puppeteer';
import fs from 'fs';
import { FORMAT, REPORT_TYPE, SELECTOR, AUTH, URL_SOURCE } from './constants.js';
import { FORMAT, AUTH} from './constants.js';
import { exit } from "process";
import ora from 'ora';

Expand Down Expand Up @@ -64,63 +64,8 @@ export async function downloadReport(url, format, width, height, filename, authT
height: height,
});

const reportSource = getReportSourceFromURL(url);

// if its an OpenSearch report, remove extra elements.
if (reportSource !== 'Other' && reportSource !== 'Saved search') {
Copy link
Member

Choose a reason for hiding this comment

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

I remember removing extra buttons was implemented to reduce unnecessary information in the report, is it ok to delete this?

If other page url (e.g. alerting) is used, reportSource would be Other so this logic and waitForSelector should be skipped. This should not make a difference?

Copy link
Member Author

@rupal-bq rupal-bq Jan 20, 2023

Choose a reason for hiding this comment

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

For alerinng it will work, but I tried anomaly detection, url: <dashboard_endpoint>/_dashboards/app/anomaly-detection-dashboards#/
Since url had dashboards#' cli got stuck waiting for #dashboardViewport

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can keep this if we have some identifiers which can't be part of other opensearch url's. I also compared the pdf report with and without this change. There's not much visible differences so I think it's ok to remove this for now. Do you have any other suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

For alerinng it will work, but I tried anomaly detection, url: <dashboard_endpoint>/_dashboards/app/anomaly-detection-dashboards#/ Since url had dashboards#' cli got stuck waiting for #dashboardViewport

You can make it matching more specific, maybe /app/dashboards#. It's ok if not much difference, did you try visualize though? I remember it removes the vis editor

Copy link
Member Author

Choose a reason for hiding this comment

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

Earlier I only tried dashboard, just tried visualize. I didn't notice much difference.
With main branch: opensearch-report-2023-01-20T22:41:33.335Z-main.pdf
With this change: opensearch-report-2023-01-20T22:39:37.417Z-fix.pdf

Copy link
Member

Choose a reason for hiding this comment

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

change the constant to

export const URL_SOURCE = {
  DASHBOARDS: "/app/dashboards#",
  VISUALIZE: "/app/visualize#",
  DISCOVER: "/app/discover#",

Copy link
Member Author

Choose a reason for hiding this comment

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

Added this change and removed waitForSelector.

await page.evaluate(
(reportSource, REPORT_TYPE) => {
// remove buttons.
document
.querySelectorAll("[class^='euiButton']")
.forEach((e) => e.remove());
// remove top navBar.
document
.querySelectorAll("[class^='euiHeader']")
.forEach((e) => e.remove());
// remove visualization editor.
if (reportSource === REPORT_TYPE.VISUALIZATION) {
document
.querySelector('[data-test-subj="splitPanelResizer"]')
?.remove();
document.querySelector('.visEditor__collapsibleSidebar')?.remove();
}
document.body.style.paddingTop = '0px';
},
reportSource,
REPORT_TYPE
);
}

// force wait for any resize to load after the above DOM modification.
await new Promise(resolve => setTimeout(resolve, 1000));

switch (reportSource) {
case REPORT_TYPE.DASHBOARD:
await page.waitForSelector(SELECTOR.DASHBOARD, {
visible: true,
});
break;
case REPORT_TYPE.VISUALIZATION:
await page.waitForSelector(SELECTOR.VISUALIZATION, {
visible: true,
});
break;
case REPORT_TYPE.NOTEBOOK:
await page.waitForSelector(SELECTOR.NOTEBOOK, {
visible: true,
});
break;
case REPORT_TYPE.DISCOVER:
await page.waitForSelector(SELECTOR.DISCOVER, {
visible: true,
});
break;
default:
break;
}

await waitForDynamicContent(page);

let buffer;
spinner.text = `Downloading Report...`;

Expand Down Expand Up @@ -198,22 +143,6 @@ const waitForDynamicContent = async (
}
};

const getReportSourceFromURL = (url) => {
if (url.includes(URL_SOURCE.DASHBOARDS)) {
return REPORT_TYPE.DASHBOARD;
}
else if (url.includes(URL_SOURCE.VISUALIZE)) {
return REPORT_TYPE.VISUALIZATION;
}
else if (url.includes(URL_SOURCE.DISCOVER)) {
return REPORT_TYPE.DISCOVER;
}
else if (url.includes(URL_SOURCE.NOTEBOOKS)) {
return REPORT_TYPE.NOTEBOOK;
}
return REPORT_TYPE.OTHER;
}

const getUrl = async (url) => {
let urlExt = url.split("#");
let urlRef = "#" + urlExt[1];
Expand Down
2 changes: 2 additions & 0 deletions test/help.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ Options:
--smtppassword <password> smtp password (env: OPENSEARCH_SMTP_PASSWORD)
--subject <subject> email Subject (default: "This is an email containing your opensearch dashboard report", env: OPENSEARCH_EMAIL_SUBJECT)
-h, --help display help for command

Note: The tenant in the url has the higher priority than tenant value provided as command option.
`;
let result = await cli(['-h'], '.');
expect(result.code).toBe(0);
Expand Down