opensearch-project / OpenSearch-Dashboards

đź“Š Open source visualization dashboards for OpenSearch.
https://opensearch.org/docs/latest/dashboards/index/
Apache License 2.0
1.68k stars 884 forks source link

[2.0] Dashboards Plugin Errors #1206

Closed boktorbb closed 2 years ago

boktorbb commented 2 years ago

This is the place to ask questions and get insights into errors you run into as you work on preparing your Plugin for 2.0. The main branch currently has the Node v14 upgrade as well as several dependency upgrades for CVE fixes. The main branch is stable and working for Core Dashboards but may not be stable for downstream plugins.

NOTE: The errors and issues posted here are not a list of things the core dashboards team will take on fix. This is a platform to engage the Core team on issues that you may need more guidance on or questions that you may have after your initial debugging work.

kavilla commented 2 years ago

Note there were some NPM packages there we upgraded to mitigate CVEs, there might be some conflict of package versions in your plugin for example: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/1146

Also make sure to update your package.json and opensearch_dashboards.json to be 2.0.0 or 2.0.0.0 in the appropriate locations for example:

For runtime, I would make sure if you have an OpenSearch plugin has a 2.0.0.0 plugin as well.

Well upgrading to 2.0.0.0, you can then add it to the build pipeline here: https://github.com/opensearch-project/opensearch-build/blob/main/manifests/2.0.0/opensearch-dashboards-2.0.0.yml

ananzh commented 2 years ago

For plugins hookup with the lastest osd with opensearch-js client, please refer to this issue .

amitgalitz commented 2 years ago

I followed the steps given in this issue for plugin hookup (Anomaly Detection). The first issue I experienced was related to package mismatch that was easy to fix. The other issues I am experiencing and not sure how to solve yet are that a lot of the AD requests from the frontend are getting a 400 bad request response. Specifically a lot of them such as http://localhost:5601/api/anomaly_detectors/detectors/test-name/_match or http://localhost:5601/api/anomaly_detectors/_indices?index= are getting this message back: "[request body]: expected value of type [any] but got [undefined]". When I directly hit the backend with the equivalent backend API there are no problems.

ananzh commented 2 years ago

Issues in AD hookup:

[1] 400 bad request

{"statusCode":400,"error":"Bad Request","message":"[request body]: expected value of type [any] but got [undefined]"}

Issue link here

Issue is due to this line here

After typescript upgrade, Dashboards use a more restrict type check. For the get API from AD, we could see body is null.

Screen Shot 2022-03-30 at 9 28 11 PM

Body null is transformed to undefined in the type check in the dashboards. Then, type undefined is no longer acceptable from type any by restricter type check.

There are two ways to change: 1) Seems request body is always null in get API. Could delete L64. 2) Change to body: schema.nullable(schema.any()),

Both would solve the issue.

[2] AnnotationDomainTypes no longer exists in @elastic/charts AnnotationDomainTypes does not exists due to @elastic/charts upgrade. This is the breaking change. AnnotationDomainType is the one used in @elastic/charts. Detector page can be rendered out.

Screen Shot 2022-03-30 at 11 13 09 PM'

Issue link here

Two ways to change: 1) a quick fix, change AnnotationDomainTypes to

 AnnotationDomainType as AnnotationDomainTypes

2) use AnnotationDomainType and modify all the places use AnnotationDomainTypes

[3] Other issues For this file /public/pages/Dashboard/Components/AnomaliesLiveChart.tsx I see there are two complains: 1) showLegendDisplayValue={false}

Type '{ showLegend: boolean; legendPosition: "right"; showLegendExtra: false; showLegendDisplayValue: boolean; xDomain: { min: number; max: number; }; }' is not assignable to type 'IntrinsicAttributes & Partial<Omit<SettingsSpec, "id" | "chartType" | "specType">> & { children?: ReactNode; }'.
  Property 'showLegendDisplayValue' does not exist on type 'IntrinsicAttributes & Partial<Omit<SettingsSpec, "id" | "chartType" | "specType">> & { children?: ReactNode; }'.ts(2322)

Could simply remove this line since it is not used.

2) LineAnnotation has no id , here

Property 'id' is missing in type '{ domainType: "xDomain"; dataValues: LineAnnotationDatum[]; style: { line: { strokeWidth: number; stroke: string; dash: number[]; opacity: number; }; }; marker: string; }' but required in type 'SpecRequiredProps'.ts(2741)
[index.d.ts(5, 5): ]()'id' is declared here.

Could add id here

                   <LineAnnotation
+                    id={'lineAnnotation'}

This issue is not a blocker.

After these changes, AD plugin can run on current Dashboards 2.0. Please help me to verify.

I could now create a detector:

Screen Shot 2022-03-30 at 9 31 08 PM

Then detector is rendered out:

Screen Shot 2022-03-30 at 11 14 13 PM
ananzh commented 2 years ago

See unit tests all fail for AD. Continue deep dive here. Some issues:

[1] test-env is missing

"The error below may be caused by using the wrong test environment, see https://jestjs.io/docs/configuration#testenvironment-string. Consider using the "jsdom" test environment."

I think probably related to we taking out the test environment in src/dev/jest/config.js and only having it in the npm package. So the solution would be adding back in jest.config.js:

testEnvironment: 'jest-environment-jsdom',

[2] wait does not exist in @testing-library/react Solution would be import waitFor from @testing-library/react, then switch from wait to waitFor. Here, waitFor requires a callback fun. If original wait() has non argument, you could pass an empty callback. For example,

await waitFor(()=>{});

[3] see some errors regarding EuiPanel

 FAIL  public/pages/Overview/containers/__tests__/AnomalyDetectionOverview.test.tsx (7.896 s)
  ● <AnomalyDetectionOverview /> spec › Some detectors created › renders component with sample detector

    TestingLibraryElementError: Unable to find an element with the text: INSTALLED. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

    Ignored nodes: comments, <script />, <style />
    <body>
      <div>
        <header
          class="euiPageHeader euiPageHeader--responsive euiPageHeader--center"
        >
          <div
            class="euiFlexGroup euiFlexGroup--gutterLarge euiFlexGroup--justifyContentSpaceBetween euiFlexGroup--directionRow euiFlexGroup--responsive"
          >
            <div
              class="euiFlexItem euiFlexItem--flexGrowZero"
            >
              <h1
                class="euiTitle euiTitle--large"
                data-test-subj="overviewTitle"
              >
                Anomaly detection
              </h1>
            </div>
            <div
              class="euiFlexItem euiFlexItem--flexGrowZero"
            >
              <a
                class="euiButton euiButton--primary euiButton--fill"
                data-test-subj="add_detector"
                href="anomaly-detection-dashboards#/create-detector-steps"
                rel="noreferrer"
              >
                <span
                  class="euiButtonContent euiButton__content"
                >
                  <span
                    class="euiButton__text"
                  >
                    Create detector
                  </span>
                </span>
              </a>
            </div>
          </div>
        </header>
        <div
          class="euiText euiText--medium"
        >
          The anomaly detection plugin automatically detects anomalies in your data in near real-time using the Random Cut Forest (RCF) algorithm.

          <a
            class="euiLink euiLink--primary"
            href="https://opensearch.org/docs/monitoring-plugins/ad"
            rel="noopener noreferrer"
            target="_blank"
          >
            Learn more 
            <svg
              aria-hidden="true"
              class="euiIcon euiIcon--small"
              focusable="false"
              height="16"
              role="img"
              viewBox="0 0 16 16"
              width="16"
              xmlns="http://www.w3.org/2000/svg"
            >
              <path
                d="M13 8.5a.5.5 0 111 0V12a2 2 0 01-2 2H4a2 2 0 01-2-2V4a2 2 0 012-2h3.5a.5.5 0 010 1H4a1 1 0 00-1 1v8a1 1 0 001 1h8a1 1 0 001-1V8.5zm-5.12.339a.5.5 0 11-.706-.707L13.305 2H10.5a.5.5 0 110-1H14a1 1 0 011 1v3.5a.5.5 0 11-1 0V2.72L7.88 8.838z"
              />
            </svg>
            <svg
              aria-label="External link"
              class="euiIcon euiIcon--small euiLink__externalIcon"
              focusable="false"
              height="16"
              role="img"
              viewBox="0 0 16 16"
              width="16"
              xmlns="http://www.w3.org/2000/svg"
            >
              <path
                d="M13 8.5a.5.5 0 111 0V12a2 2 0 01-2 2H4a2 2 0 01-2-2V4a2 2 0 012-2h3.5a.5.5 0 010 1H4a1 1 0 00-1 1v8a1 1 0 001 1h8a1 1 0 001-1V8.5zm-5.12.339a.5.5 0 11-.706-.707L13.305 2H10.5a.5.5 0 110-1H14a1 1 0 011 1v3.5a.5.5 0 11-1 0V2.72L7.88 8.838z"
              />
            </svg>
            <span
              class="euiScreenReaderOnly"
            >
              (opens in a new tab or window)
            </span>
          </a>
        </div>
        <div
          class="euiSpacer euiSpacer--xl"
        />
        <div
          class="euiPanel euiPanel--paddingMedium euiPanel--borderRadiusMedium euiPanel--plain euiPanel--hasShadow"
          style="padding: 20px;"
        >
          <div
            class="euiFlexGroup euiFlexGroup--gutterLarge euiFlexGroup--alignItemsCenter euiFlexGroup--justifyContentSpaceBetween euiFlexGroup--directionRow euiFlexGroup--responsive"
            style="padding: 0px;"
          >
            <div
              class="euiFlexItem"
            >
              <h3
                class="euiTitle euiTitle--small"
                data-test-subj="contentPanelTitle"
              >
                How it works
              </h3>
              <div
                class="euiFlexGroup euiFlexGroup--gutterLarge euiFlexGroup--directionRow euiFlexGroup--responsive"
              >
                <div
                  class="euiFlexItem content-panel-subTitle"
                  style="line-height: normal; max-width: 75%;"
                />
              </div>
            </div>
            <div
              class="euiFlexItem euiFlexItem--flexGrowZero"
            >
              <div
                class="euiFlexGroup euiFlexGroup--gutterMedium euiFlexGroup--alignItemsCenter euiFlexGroup--justifyContentSpaceBetween euiFlexGroup--directionRow euiFlexGroup--responsive"
              >
                <div
                  class="euiFlexItem"
                />
              </div>
            </div>
          </div>
          <div>
            <hr
              class="euiHorizontalRule euiHorizontalRule--full euiHorizontalRule--marginSmall"
            />
            <div
              style="padding: 10px 0px;"
            >
              <div
                class="euiFlexGroup euiFlexGroup--gutterLarge euiFlexGroup--directionRow euiFlexGroup--responsive"
              >
                <div
                  class="euiFlexItem"
                >
                  <div
                    class="euiFlexGroup euiFlexGroup--gutterLarge euiFlexGroup--directionColumn euiFlexGroup--responsive"
                  >
                    <div
                      class="euiFlexItem euiFlexItem--flexGrowZero"
                      style="margin-bottom: 5px;"
                    >
                      <div
                        class=...

       99 |       expect(getAllByText('Detector created')).toHaveLength(1);
      100 |       expect(getAllByText('View detector and sample data')).toHaveLength(1);
    > 101 |       expect(getAllByText('INSTALLED')).toHaveLength(1);
          |              ^
      102 |     });
      103 |     test('renders component with non-sample detector', async () => {
      104 |       httpClientMock.get = jest.fn().mockResolvedValue({

      at Object.getElementError (../../node_modules/@testing-library/dom/dist/config.js:38:19)
      at ../../node_modules/@testing-library/dom/dist/query-helpers.js:90:38
      at getAllByText (../../node_modules/@testing-library/dom/dist/query-helpers.js:130:20)
      at _callee2$ (public/pages/Overview/containers/__tests__/AnomalyDetectionOverview.test.tsx:101:14)
      at tryCatch (node_modules/@babel/runtime/node_modules/regenerator-runtime/runtime.js:63:40)
      at Generator.invoke [as _invoke] (node_modules/@babel/runtime/node_modules/regenerator-runtime/runtime.js:293:22)
      at Generator.next (node_modules/@babel/runtime/node_modules/regenerator-runtime/runtime.js:118:21)
      at asyncGeneratorStep (node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:24)
      at _next (node_modules/@babel/runtime/helpers/asyncToGenerator.js:25:9)

Two issues here: 1) style has some incompatibilities

Type '{ alignContent?: Property.AlignContent | undefined; alignItems?: Property.AlignItems | undefined; alignSelf?: Property.AlignSelf | undefined; ... 780 more ...; vectorEffect?: Property.VectorEffect | undefined; }' is not assignable to type 'Properties<string | number, string & {}>'.
  Types of property 'MozForceBrokenImageIcon' are incompatible.
    Type 'import("/home/anan/work/OpenSearch-Dashboards/plugins/anomaly-detection-dashboards-plugin/node_modules/csstype/index").Property.MozForceBrokenImageIcon | undefined' is not assignable to type 'import("/home/anan/work/OpenSearch-Dashboards/node_modules/@types/react/node_modules/csstype/index").Property.MozForceBrokenImageIcon | undefined'.
      Type 'number & {}' is not assignable to type 'MozForceBrokenImageIcon | undefined'.
        Type 'number & {}' is not assignable to type '1'.ts(2322)
[index.d.ts(1829, 9): ]()The expected type comes from property 'style' which is declared here on type 'IntrinsicAttributes & PropsWithChildren<EuiCardProps>'

Solution would be simplify style and remove un-match parts. For example here, change to:

style={{ padding: '20px'}}

2) Property 'betaBadgeLabel' does not exist on EuiPanel

Type '{ children: (Element | null)[]; style: { padding: string; }; className: string | undefined; betaBadgeLabel: string | undefined; }' is not assignable to type 'IntrinsicAttributes & PropsWithChildren<(DisambiguateSet<_EuiPanelButtonlike, _EuiPanelDivlike> & _EuiPanelDivlike) | (DisambiguateSet<...> & _EuiPanelButtonlike)>'.
  Property 'betaBadgeLabel' does not exist on type 'IntrinsicAttributes & PropsWithChildren<(DisambiguateSet<_EuiPanelButtonlike, _EuiPanelDivlike> & _EuiPanelDivlike) | (DisambiguateSet<...> & _EuiPanelButtonlike)>'.ts(2322)

For this one, I made an ugly fix. Not sure whether this is the best solution. I see betaBadgeLabel is a property in EuiCard and only EuiCard supports it. Meanwhile, EuiPanel is a building block component. Use it as a layout helper for containing content. It is used as a base for EuiCard. If betaBadgeLabel is required, I think we could change to EuiCard. Otherwise, could reconstruct this component. I just replace EuiPanel to EuiCard for ContentPanel component here.

const ContentPanel = (props: ContentPanelProps) => {
  const titleDataTestSubj = get(
    props,
    'titleDataTestSubj',
    'contentPanelTitle'
  );
  return (
    <EuiCard
      style={{ padding: '20px'}}
      title={""}
      className={props.contentPanelClassName}
      betaBadgeLabel={props.badgeLabel}
    >
      <EuiFlexGroup
        style={{ padding: '0px' }}
        justifyContent="spaceBetween"
        alignItems="center"
      >
        <EuiFlexItem>
          {typeof props.title === 'string' ? (
            <EuiTitle
              data-test-subj={titleDataTestSubj}
              size={props.titleSize || 's'}
              className={props.titleClassName}
            >
              <h3>{props.title}</h3>
            </EuiTitle>
          ) : (
            <EuiFlexGroup
              data-test-subj={titleDataTestSubj}
              justifyContent="flexStart"
              alignItems="center"
            >
              {Array.isArray(props.title) ? (
                props.title.map(
                  (titleComponent: React.ReactNode, idx: number) => (
                    <EuiFlexItem grow={false} key={idx}>
                      {titleComponent}
                    </EuiFlexItem>
                  )
                )
              ) : (
                <EuiFlexItem>{props.title}</EuiFlexItem>
              )}
            </EuiFlexGroup>
          )}
          <EuiFlexGroup>
            {Array.isArray(props.subTitle) ? (
              props.subTitle.map(
                (subTitleComponent: React.ReactNode, idx: number) => (
                  <EuiFlexItem
                    grow={false}
                    key={idx}
                    className="content-panel-subTitle"
                    style={{ lineHeight: 'normal', maxWidth: '75%' }}
                  >
                    {subTitleComponent}
                  </EuiFlexItem>
                )
              )
            ) : (
              <EuiFlexItem
                className="content-panel-subTitle"
                style={{ lineHeight: 'normal', maxWidth: '75%' }}
              >
                {props.subTitle}
              </EuiFlexItem>
            )}
          </EuiFlexGroup>
        </EuiFlexItem>

        <EuiFlexItem grow={false}>
          <EuiFlexGroup
            justifyContent="spaceBetween"
            alignItems="center"
            gutterSize="m"
          >
            {Array.isArray(props.actions) ? (
              props.actions.map((action: React.ReactNode, idx: number) => (
                <EuiFlexItem key={idx}>{action}</EuiFlexItem>
              ))
            ) : (
              <EuiFlexItem>{props.actions}</EuiFlexItem>
            )}
          </EuiFlexGroup>
        </EuiFlexItem>
      </EuiFlexGroup>
      {!isEmpty(props.actions) ? <EuiSpacer size="s" /> : null}
      {props.title != '' && props.hideBody !== true ? (
        <div>
          <EuiHorizontalRule
            margin="s"
            className={props.horizontalRuleClassName}
          />
          <div style={{ padding: '10px 0px' }}>
            {props.children}
          </div>
        </div>
      ) : null}
    </EuiCard>
  );
};

After all these changes, all unit tests passed:

Screen Shot 2022-03-31 at 2 53 06 PM

UI seems working

Screen Shot 2022-03-31 at 2 53 29 PM Screen Shot 2022-03-31 at 2 53 41 PM

Again, not a UI expert, so I think my fix is a bit tmp and ugly. Just want to research on this and see they could be fixed or not. There might be other better solutions.

tmarkley commented 2 years ago

We are pushing through the following PRs to address which may require changes for plugins:

https://github.com/opensearch-project/OpenSearch-Dashboards/pull/1379 https://github.com/opensearch-project/OpenSearch-Dashboards/pull/1449 https://github.com/opensearch-project/OpenSearch-Dashboards/pull/1451 https://github.com/opensearch-project/OpenSearch-Dashboards/pull/1456

CEHENKLE commented 2 years ago

@tmarkley If those are all merged, can we close this issue?

tmarkley commented 2 years ago

@CEHENKLE We’re using this as a place for plugins to reach out if they have issues with 2.0 changes. Those PRs were just a few of the recent changes that we wanted to highlight. Our plan was to keep this open for visibility until we release 2.0.

CEHENKLE commented 2 years ago

Cool beans, thanks :)

lguillaud commented 2 years ago

@kavilla Done for https://github.com/lguillaud/osd_transform_vis/issues/1 (2.0.0-rc1)