Skip to content

Conversation

@kaituo
Copy link
Collaborator

@kaituo kaituo commented Jun 3, 2025

Description

The PR covers creating and managing forecasting jobs from a data source and consuming forecasting results visualized on a graph. Forecasting will show the user where their metrics are headed so that they can prepare accordingly (e.g. disk space, server instance upgrades, inventory planning).

Testing done:

  1. added IT for create/edit/list forecasters: kaituo/opensearch-dashboards-functional-test@d89f945#diff-3c1decf4ad1bc8b8ce0c2aad9e09a282a70b0284e95c1622d571ad46a8506d16
  2. added UTs. Will add more later.

Issues Resolved

#577
#576
#575
#574
#573
#441

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

{/* HISTORY */}
<Field name="history" validate={validatePositiveInteger}>
{({ field, form }: FieldProps<number>) => {
const interval = (form as FormikProps<any>).values.interval;
Copy link
Collaborator

@jackiehanyang jackiehanyang Jun 9, 2025

Choose a reason for hiding this comment

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

nit: maybe try to define the full form type once to avoid casting it as . Leaving this as a nit comment because only seeing usage for this interface twice so far. If we're using it somewhere else or in the future we're going to use it again, definitely consider to define an interface for this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for your understanding.

await dispatch(getMappings(selectedIndices, dataSourceId));
}
};
console.log('forecasterId', forecasterId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove console.log code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

: 'history,horizon,window_delay';

try {
console.log('forecasterBody', forecasterBody);
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove console.log code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

sliderPanelRef.current &&
!sliderPanelRef.current.contains(e.target as Node)
) {
console.log('Outside slider => Hiding slider');
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove console.log code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

expect(validateQuery('{"a":{"b":{}}}')).toBeUndefined();
});
test('should return error message if invalid query', () => {
console.log = jest.fn();
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove console.log code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

try {
JSON.parse(value);
} catch (err) {
console.log('Returning error', err);
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove console log code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Logging the error would help debug production issues.

Comment on lines 311 to 322
onChange={(e) => {
const raw = e.target.value;
form.setFieldValue(field.name, raw === '' ? '' : Number(raw));
}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: seeing repeat onChange blocks with identical logic multiple times in this file. Maybe consider to extract it out as a common method

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

extracted a method:

const toNumberOrEmpty = (value: string): number | '' => {
  return value === '' ? '' : Number(value);
};

Comment on lines 312 to 322
onChange={(e) => {
const raw = e.target.value;
form.setFieldValue(field.name, raw === '' ? '' : Number(raw));
}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as in the AdvancedSetting file above, the onChange method here is identical and repeated multiple times

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed

>
<EuiFlexItem grow={false}>
<Field
name={`imputationOption.custom_value.${index}.data`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

from line 258, it seems like we don't have data key in imputationOption.custom_value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What is the data key you are referring to?

Comment on lines +102 to +103
// FIXME: EuiRadio doesn't support readOnly prop. readOnly would be preferred as disabled makes the radio look gray.
// Consider creating a custom radio component that supports readOnly if this styling is important.
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we clean up this comment?

Copy link
Collaborator Author

@kaituo kaituo Jun 10, 2025

Choose a reason for hiding this comment

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

no need, it is still relevant

@@ -0,0 +1,289 @@
import React, { useState } from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing license session

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

@@ -0,0 +1,331 @@
import React, { Fragment, useEffect, useRef, useState } from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing license header

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

@@ -0,0 +1,74 @@
import React from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing license header

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

@@ -0,0 +1,337 @@
import { useEffect, useState } from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing license header

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

}
});
});
} catch (e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

empty catch(e) block?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added catch block

Signed-off-by: Kaituo Li <[email protected]>
Signed-off-by: Kaituo Li <[email protected]>
@codecov
Copy link

codecov bot commented Jun 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.67%. Comparing base (05c55bf) to head (3c9a534).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1038      +/-   ##
==========================================
- Coverage   53.81%   48.67%   -5.15%     
==========================================
  Files         170      232      +62     
  Lines        6955    10630    +3675     
  Branches     1430     2277     +847     
==========================================
+ Hits         3743     5174    +1431     
- Misses       2783     4924    +2141     
- Partials      429      532     +103     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kaituo
Copy link
Collaborator Author

kaituo commented Jun 10, 2025

Install Dashboard with Plugin fails with Run OpenSearch. Looks like a problem of backend image issue:

github flow:

- name: Run Opensearch
        uses: derek-ho/start-opensearch@v2
        with:
          opensearch-version: ${{ env.OPENSEARCH_VERSION }}
          security-enabled: false

CI log:

 * connect to ::1 port 9200 from ::1 port 38724 failed: Connection refused
*   Trying 127.0.0.1:9200...
* connect to 127.0.0.1 port 9200 from 127.0.0.1 port 60638 failed: Connection refused
* Failed to connect to localhost port 9200 after 0 ms: Couldn't connect to server

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
* Closing connection
curl: (7) Failed to connect to localhost port 9200 after 0 ms: Couldn't connect to server
Error: Process completed with exit code 7.
Run cat ./opensearch-3.1.0-SNAPSHOT/logs/opensearch.log
  cat ./opensearch-3.1.0-SNAPSHOT/logs/opensearch.log
  shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
  env:
    OPENSEARCH_VERSION: 3.1.0
    CI: 1
    TERM: xterm
    PLUGIN_VERSION: 3.1.0.0
    JAVA_HOME_11.0.27_x64: /opt/hostedtoolcache/jdk/11.0.27/x64
    JAVA_HOME: /opt/hostedtoolcache/jdk/11.0.27/x64
    JAVA_HOME_11_0_27_X64: /opt/hostedtoolcache/jdk/11.0.27/x64
cat: ./opensearch-3.1.0-SNAPSHOT/logs/opensearch.log: No such file or directory
Error: Process completed with exit code 1.

@kaituo
Copy link
Collaborator Author

kaituo commented Jun 10, 2025

codecov failure: will more tests later. Currently prioritizing IT that codecov does not consider: kaituo/opensearch-dashboards-functional-test@d89f945#diff-3c1decf4ad1bc8b8ce0c2aad9e09a282a70b0284e95c1622d571ad46a8506d16

throw get(result, 'error', '');
}
} catch (error) {
console.log('Processed error in middleware:', error);
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove console.log

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This would help prod failure.

items.length > 0
? groupIndicesOrAliasesByCluster(items, localClusterName, label)
: [{ label, options: items }];
console.log('getVisibleOptions', indices, aliases, localClusterName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove console.log

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

Signed-off-by: Kaituo Li <[email protected]>
@kaituo kaituo merged commit a1657ec into opensearch-project:main Jun 10, 2025
10 of 17 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1038-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 a1657ecce6839a1573c70bb42aba2400cfd741a5
# Push it to GitHub
git push --set-upstream origin backport/backport-1038-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-1038-to-2.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants