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

[CCI] Add bluebird replaces for src/plugins/vis_type_timeline #4025

Closed

Conversation

Nicksqain
Copy link
Contributor

Description

Issues Resolved

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
@codecov
Copy link

codecov bot commented May 14, 2023

Codecov Report

Merging #4025 (87d9a82) into main (574f119) will decrease coverage by 0.11%.
The diff coverage is 76.47%.

@@            Coverage Diff             @@
##             main    #4025      +/-   ##
==========================================
- Coverage   66.46%   66.35%   -0.11%     
==========================================
  Files        3229     3229              
  Lines       62091    62137      +46     
  Branches     9607     9619      +12     
==========================================
- Hits        41269    41234      -35     
- Misses      18511    18581      +70     
- Partials     2311     2322      +11     
Flag Coverage Δ
Linux 66.30% <76.47%> (-0.11%) ⬇️
Windows 66.30% <76.47%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/osd-dev-utils/src/proc_runner/proc.ts 6.52% <0.00%> (ø)
packages/osd-pm/src/utils/fs.ts 13.04% <ø> (-20.29%) ⬇️
src/core/public/core_app/status/status_app.tsx 0.00% <0.00%> (ø)
src/core/public/doc_links/doc_links_service.ts 100.00% <ø> (ø)
src/core/server/bootstrap.ts 0.00% <0.00%> (ø)
src/core/server/plugins/types.ts 100.00% <ø> (ø)
src/core/server/status/status_service.ts 78.94% <ø> (ø)
...components/create_form/create_data_source_form.tsx 74.00% <ø> (ø)
...tion/server/collectors/application_usage/schema.ts 100.00% <ø> (ø)
..._collection/server/collectors/management/schema.ts 100.00% <ø> (ø)
... and 24 more

... and 23 files with indirect coverage changes

Nicksqain added 2 commits May 14, 2023 14:52
Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
@abbyhu2000 abbyhu2000 added the OSCI Open Source Contributor Initiative label May 16, 2023
ananzh and others added 2 commits May 26, 2023 08:27
Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
@@ -106,7 +105,8 @@ export function runRoute(
savedObjectsClient: context.core.savedObjects.client,
});
const chainRunner = chainRunnerFn(tlConfig);
const sheet = await Bluebird.all(chainRunner.processRequest(request.body));
const promises = chainRunner.processRequest(request.body);
const sheet = await Promise.all(Array.from(promises));
Copy link
Member

Choose a reason for hiding this comment

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

Does chainRunner.processRequest(request.body) returns returns an array of promises? or it returns an iterable (like a Set) of promises, so you convert it to an array before passing it to Promise.all?

From chainRunnerFn def, I see the return is

return {
    processRequest: processRequest,
    getStats: function () {
      return stats;
    },
  };

where processRequest is defined as

function processRequest(request) {
    if (!request) throw new Error('Empty request body');

    validateTime(request.time, tlConfig);

    tlConfig.time = request.time;
    tlConfig.time.to = moment(request.time.to).valueOf();
    tlConfig.time.from = moment(request.time.from).valueOf();
    tlConfig.time.interval = calculateInterval(
      tlConfig.time.from,
      tlConfig.time.to,
      tlConfig.settings['timeline:target_buckets'] || 200,
      tlConfig.time.interval,
      tlConfig.settings['timeline:min_interval'] || '1ms'
    );

    tlConfig.setTargetSeries();

    stats.invokeTime = new Date().getTime();
    stats.queryCount = 0;
    queryCache = {};

    // This is setting the "global" sheet, required for resolving references
    sheet = parseSheet(request.sheet);
    return preProcessSheet(sheet).then(function () {
      return _.map(sheet, function (chainList, i) {
        return resolveChainList(chainList)
          .then(function (seriesList) {
            stats.sheetTime = new Date().getTime();
            return seriesList;
          })
          .catch(function (e) {
            throwWithCell(i, e);
          });
      });
    });
  }

The _.map function from lodash is being used here.resolveChainList(chainList) returns a Promise. _.map returns an array. So it seems to me the return type is array of Promise. Therefore, we could do

const sheet = await Promise.all(chainRunner.processRequest(request.body));

?

Could you help me to double check and verify? Could set some console logs to verify the type of promises chainRunner returns.

Since this is in server, you could add launch.json in .vscode then add a break point when you run a timeline func

{
    "version": "0.2.0",
    "configurations": [
        {
            "type": "pwa-node",
            "request": "attach",
            "name": "back",
            "port": 9229
        }
    ]
}

Copy link
Member

@ananzh ananzh left a comment

Choose a reason for hiding this comment

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

Overall nice and clean change! Thank you so much. I have two suggestions here:

  • Help to verify the return type const promises = chainRunner.processRequest(request.body); Pls see my comment.
  • Add CHANGELOG.

@Nicksqain
Copy link
Contributor Author

Nicksqain commented May 26, 2023

@ananzh
Thanks for the reply!
Should the .vscode folder be in the server folder? Or in the root of the project? I can't get the debugger to run.
How can I run this function if it is tied to an api endpoint?

AMoo-Miki and others added 5 commits May 26, 2023 11:40
* Bump Node.js requirements to 18

Signed-off-by: Miki <miki@amazon.com>

* Replace `lmdb-store` with `lmdb`

Signed-off-by: Miki <miki@amazon.com>
Co-authored-by: Anan Zhuang <ananzh@amazon.com>

* Bump `elastic-apm-node` to the latest minor

Signed-off-by: Miki <miki@amazon.com>

* Replace webpack and plugins with a patched version that uses xxhash64
* Use `xxhash64` as the hashing algorithm of webpack
* Upgrade `globby`
* Remove `fibers`

Signed-off-by: Miki <miki@amazon.com>

* Replace `fs.rmdir` with `fs.rm` in cross-platform tests

Signed-off-by: Miki <miki@amazon.com>

* Increase listener limit

Signed-off-by: Miki <miki@amazon.com>
Co-authored-by: Anan Zhuang <ananzh@amazon.com>

* Add promise-stripping serializer

Signed-off-by: Miki <miki@amazon.com>

* Bump heap for CI

Signed-off-by: Miki <miki@amazon.com>

* Correct use of fs/promises in @osd/pm

Signed-off-by: Miki <miki@amazon.com>

* Use fs/promise in plugin post-install cleanup

Signed-off-by: Miki <miki@amazon.com>

* Set the test server's host to `0.0.0.0`

Signed-off-by: Miki <miki@amazon.com>

* Sync `.node-version` file

Signed-off-by: Miki <miki@amazon.com>

* Support both `isPrimary`, for Node 18, and `isMaster`, for Node 14

Signed-off-by: Miki <miki@amazon.com>

* Add types when using `isDeepStrictEqual`

Signed-off-by: Miki <miki@amazon.com>
Co-authored-by: Anan Zhuang <ananzh@amazon.com>

* Add names to `SchemaError` to log more specific errors

Signed-off-by: Miki <miki@amazon.com>

* Fix failing vega visualization tests outside the CI

Signed-off-by: Miki <miki@amazon.com>

* Fix snapshot of errors thrown for undefined accessors

Signed-off-by: Miki <miki@amazon.com>

* Fix flakiness of log_rotator

Signed-off-by: Miki <miki@amazon.com>

* Fix asynchronous `fs` usafe in plugin discover

Signed-off-by: Miki <miki@amazon.com>

* Fix mocks in @osd/optimizer

Signed-off-by: Miki <miki@amazon.com>

* Fix memory leaks caused by setting states on unloaded components

Signed-off-by: Miki <miki@amazon.com>
Co-authored-by: Anan Zhuang <ananzh@amazon.com>

* Bump Node in Dockerfile

Signed-off-by: Miki <miki@amazon.com>
Co-authored-by: Anan Zhuang <ananzh@amazon.com>

* Remove the response `close` event as an indicator of the requesting finishing

opensearch-project#3601 (comment)

Signed-off-by: Miki <miki@amazon.com>
Co-authored-by: Anan Zhuang <ananzh@amazon.com>

* [BWC] Timeout after 3 mins of waiting for OSD to be running in tests

Signed-off-by: Miki <miki@amazon.com>

* Make build use the same node version that tests are run against

Signed-off-by: Miki <miki@amazon.com>

* Make Node resolve DNS by IPv4 first
* This is helpful to resolve `locahost` to `127.0.0.1`

Signed-off-by: Miki <miki@amazon.com>

* Standardize patterns used by plugin discovery
* Enhance absolute path serialization on  Windows

Signed-off-by: Miki <amoo_miki@yahoo.com>

* Mock fetch in SenseEditor tests

Signed-off-by: Miki <amoo_miki@yahoo.com>

* Restore node-sass usage to fix build performance

* `sass-loader@10` is the last version that supports webpack@4
* `sass` is extremely slow when using the legacy API (`render`) and to use the "Modern API" (`compileStringAsync`), `sass-loader@13` would be needed.
* The performance of `sass@10` is made acceptable only with `fibers` but that is deprecated and doesn't work on Node 18

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Signed-off-by: Miki <miki@amazon.com>

* Revert "[CI] setup Chrome and utilize binary path (opensearch-project#3997)"

This reverts commit 0188d05

Signed-off-by: Miki <miki@amazon.com>

* Prevent fast-fail while running functional test in CI

Signed-off-by: Miki <miki@amazon.com>

* Revert "Temporarily hardcode chromedriver to 112.0.0 to enable all ftr tests (opensearch-project#3976)"

This reverts commit 5ea0cbe.

Signed-off-by: Miki <miki@amazon.com>

* Save Cypress results artifacts during CI

Signed-off-by: Miki <miki@amazon.com>

* Add missing required dependency on `set-value`

* Also force all to ^4.1.0 due to a vulnerability fixed in 3.1.0.

Signed-off-by: Miki <miki@amazon.com>

* Prevent multiple calls to bootstrap's shutdown

Signed-off-by: Miki <miki@amazon.com>

* Use Node 18.16.0 in distributions

* Bump jest-canvas-mock to fix failing tests
* Extend Node engines versions

Signed-off-by: Miki <miki@amazon.com>

* Normalize test snapshots across Node 14, 16, and 18

Signed-off-by: Miki <miki@amazon.com>

* Update CHANGELOG for Node.js >=14.20.1 <19 support

Signed-off-by: Miki <miki@amazon.com>

---------

Signed-off-by: Miki <miki@amazon.com>
Signed-off-by: Miki <amoo_miki@yahoo.com>
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Co-authored-by: Anan Zhuang <ananzh@amazon.com>
* Remove timeline application

In this PR, we made the following changes:
First of all, clean out some advanced settings specific to timeline
application and tests.
* Remove timelion:default_rows: This setting defines the default
number of rows that a new Timelion sheet should have.
* Remove timelion:default_rows: This setting defines the default
number of columns that a new Timelion sheet should have.
* Remove timelion:showTutorial.

Second, remove src/plugin/timeline completely and modify timeline vis.
Third, remove all the functional tests related to timeline application.

Issue resolve
opensearch-project#3519
opensearch-project#3593

Signed-off-by: ananzh <ananzh@amazon.com>

---------

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Signed-off-by: ananzh <ananzh@amazon.com>
…de 18 (opensearch-project#4151)

Signed-off-by: ananzh <ananzh@amazon.com>
Signed-off-by: Miki <miki@amazon.com>
CHANGELOG.md Outdated
@@ -204,6 +204,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Console] Remove unused ul element and its custom styling ([#3993](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3993))
- Fix EUI/OUI type errors ([#3798](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3798))
- Remove unused Sass in `tile_map` plugin ([#4110](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4110))
- Add bluebird replaces for vis_type_timeline plugin ([#4025](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4025))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Add bluebird replaces for vis_type_timeline plugin ([#4025](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4025))
- Replace the use of `bluebird` in the `vis_type_timeline` plugin ([#4025](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4025))

zhongnansu and others added 4 commits May 31, 2023 11:39
…#4188)

Snapshot checksum verification caused failure in test runs:
opensearch-project#4187

Skipping the verification to enable the tests run as the snapshot
of OpenSearch should not impact the tests.

Issue:
n/a

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
…d verifies if it is installed (opensearch-project#3116)

Resolves Issue -opensearch-project#2799

Signed-off-by: Manasvini B Suryanarayana <manasvis@amazon.com>
…favor of OUI utility class. (opensearch-project#4164)

* remove custom styling in favor of oui utility class

Signed-off-by: Sirazh Gabdullin <sirazh.gabdullin@nu.edu.kz>

* Update CHANGELOG.md

Signed-off-by: Sirazh Gabdullin <sirazh.gabdullin@nu.edu.kz>

---------

Signed-off-by: Sirazh Gabdullin <sirazh.gabdullin@nu.edu.kz>
@ananzh
Copy link
Member

ananzh commented Jun 1, 2023

@Nicksqain

Screenshot 2023-06-01 at 09 02 05

@ananzh
Copy link
Member

ananzh commented Jun 1, 2023

@Nicksqain I think const promises = chainRunner.processRequest(request.body); should return <Promise<Array<Promise<some data type>>>. Therefore, const sheet = await Promise.all(chainRunner.processRequest(request.body)); is more appropriate here. Feel free to verify and pls resolve the CHANGELOG conflict and rebase to let this PR tested with our latest changes.

@ananzh ananzh added v2.9.0 backport 2.x technical debt If not paid, jeapardizes long-term success and maintainability of the repository. labels Jun 1, 2023
@Nicksqain
Copy link
Contributor Author

@Nicksqain I think const promises = chainRunner.processRequest(request.body); should return <Promise<Array<Promise<some data type>>>. Therefore, const sheet = await Promise.all(chainRunner.processRequest(request.body)); is more appropriate here. Feel free to verify and pls resolve the CHANGELOG conflict and rebase to let this PR tested with our latest changes.

@ananzh
At first I thought so too, but because I didn't put in the changes I saw that there was an error, checked the type and it really didn't match the iterable type
изображение

Nicksqain added 5 commits June 1, 2023 22:44
Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
@Nicksqain
Copy link
Contributor Author

@ananzh My git is broken, I'm sorry. how do I roll back my rebase?

@ananzh
Copy link
Member

ananzh commented Jun 1, 2023

@Nicksqain A simple way is to pull the latest main, then create a new branch and cherry-pick the commit here. Then force push.

@ananzh
Copy link
Member

ananzh commented Jun 1, 2023

const sheet = await Promise.all(chainRunner.processRequest(request.body));

@Nicksqain I don't what causes the error in your screenshot, but I tried to use const sheet = await Promise.all(chainRunner.processRequest(request.body)); and can't reproduce your error. Here is the commit:
main...ananzh:OpenSearch-Dashboards:timeline-bluebird

Here is the function test group 1 result:
https://github.com/ananzh/OpenSearch-Dashboards/actions/runs/5148602476/jobs/9270514321

 └-: timeline app
         └-> "before all" hook in "timeline app"
         └-> "before all" hook in "timeline app"
         └-: expression typeahead
           └-> "before all" hook for "should display function suggestions filtered by function name"
           └-> "before all" hook for "should display function suggestions filtered by function name"
             │ proc [opensearch-dashboards]   log   [20:06:50.309] [info][savedobjects-service] Creating index .kibana_2.
             │ proc [opensearch-dashboards]   log   [20:06:50.369] [info][savedobjects-service] Reindexing .kibana to .kibana_1
             │ proc [opensearch-dashboards]   log   [20:06:50.724] [info][savedobjects-service] Migrating .kibana_1 saved objects to .kibana_2
             │ proc [opensearch-dashboards]   log   [20:06:50.728] [info][savedobjects-service] Pointing alias .kibana to .kibana_2.
             │ proc [opensearch-dashboards]   log   [20:06:50.743] [info][savedobjects-service] Finished in 435ms.
             │ERROR browser[SEVERE] http://localhost:6610/9007199254740991/bundles/core/core.entry.js 20496:18 TypeError: Failed to fetch
             │          at Fetch.fetchResponse (http://localhost:6610/9007199254740991/bundles/core/core.entry.js:21071:13)
             │          at async interceptResponse (http://localhost:6610/9007199254740991/bundles/core/core.entry.js:21574:10)
             │          at async http://localhost:6610/9007199254740991/bundles/core/core.entry.js:20982:39
           └-> should display function suggestions filtered by function name
             └-> "before each" hook: global before each for "should display function suggestions filtered by function name"
             └- ✓ pass  (468ms) "timeline app expression typeahead should display function suggestions filtered by function name"
           └-> should show argument suggestions when function suggestion is selected
             └-> "before each" hook: global before each for "should show argument suggestions when function suggestion is selected"
             └- ✓ pass  (1.6s) "timeline app expression typeahead should show argument suggestions when function suggestion is selected"
           └-> should show argument value suggestions when argument is selected
             └-> "before each" hook: global before each for "should show argument value suggestions when argument is selected"
             └- ✓ pass  (2.7s) "timeline app expression typeahead should show argument value suggestions when argument is selected"
           └-: dynamic suggestions for argument values
             └-: .opensearch()
               └-> should show index pattern suggestions for index argument
               └-> should show field suggestions for timefield argument when index pattern set
               └-> should show field suggestions for split argument when index pattern set
               └-> should show field suggestions for metric argument when index pattern set
           └-> "after all" hook for "should show argument value suggestions when argument is selected"
         └-> "after all" hook in "timeline app"
     │

Note: I removed the bootstrap for windows. so windows failure is fine. See linux passed all ftr tests.

@joshuarrrr joshuarrrr added the needs more info Requires more information from poster label Jun 13, 2023
@ananzh ananzh added v2.10.0 and removed v2.9.0 labels Jul 5, 2023
@abbyhu2000 abbyhu2000 marked this pull request as draft July 25, 2023 19:14
@ananzh
Copy link
Member

ananzh commented Jul 25, 2024

@Nicksqain Due to an extended period without updates, we're going to close this issue for now. We appreciate your contribution and understand that priorities and availability can change. Please feel free to reopen this issue when you have the opportunity to revisit it. Thank you for your understanding.

@ananzh ananzh closed this Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more info Requires more information from poster OSCI Open Source Contributor Initiative repeat-contributor technical debt If not paid, jeapardizes long-term success and maintainability of the repository.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eliminate Bluebird in favor of native async functionality
9 participants