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

[TSVB] Fix TSVB is not reporting all categories of Elasticsearch error #102926

Merged
merged 5 commits into from
Jun 30, 2021

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Jun 22, 2021

Closes: #94182, Closes: #74541

Summary

TSVB is not reporting all categories of Elasticsearch error

Steps to reproduce:

  1. Go to TSVB and set the time range to Last 30 days
  2. Change the time interval to 1 minute/60 seconds, and you'll see this error and a blank chart:

Screen Shot 2021-03-09 at 2 42 09 PM

  1. Go to the Kibana advanced settings and increase the metrics:max_buckets limit from 2000 to 100000, which is higher than Elasticsearch allows

  2. Go back to TSVB and choose "Last 30 days" with an interval of 30 seconds.

  3. After issuing the query, TSVB will show a "No results" screen, but no error message. The error is completely silent.

Screen:

image

@alexwizp alexwizp self-assigned this Jun 22, 2021
@alexwizp alexwizp added v7.14.0 v8.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix labels Jun 22, 2021
@@ -147,16 +145,6 @@ function TimeseriesVisualization({
handlers.done();
});

// Show the error panel
const error = isVisSeriesData(visData) && visData[model.id]?.error;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dead code, it will never be called

@@ -159,8 +159,8 @@ export class VisEditor extends Component<TimeseriesEditorProps, TimeseriesEditor
this.setState({ autoApply: event.target.checked });
};

onDataChange = ({ visData }: { visData: TimeseriesVisData }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in case of error it's null with an error in console

@alexwizp alexwizp marked this pull request as ready for review June 23, 2021 08:54
@alexwizp alexwizp requested a review from a team June 23, 2021 08:54
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@VladLasitsa VladLasitsa left a comment

Choose a reason for hiding this comment

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

Can we move validateInterval from src\plugins\vis_type_timeseries\public\request_handler.ts to the server side? Because as I see it works incorrectly now, we check only panel interval but ignore series interval which can provide user.

Copy link
Contributor

@VladLasitsa VladLasitsa left a comment

Choose a reason for hiding this comment

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

LGTM, tested locally, thank you!

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeTimeseries 530 529 -1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
visTypeTimeseries 979.6KB 982.7KB +3.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
visTypeTimeseries 28.6KB 24.5KB -4.1KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @alexwizp

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Code LGTM, I tested some scenarios and I think that it works fine. Many errors that were not displayed (TSVB appeared with No results) are reported now. I think that this is very important for our users' experience!

@wylieconlon I would appreciate it if you could also take a look!

@stratoula stratoula requested a review from wylieconlon June 28, 2021 12:15
@alexwizp alexwizp merged commit 790bd35 into elastic:master Jun 30, 2021
alexwizp added a commit to alexwizp/kibana that referenced this pull request Jun 30, 2021
elastic#102926)

* [TSVB] Fix TSVB is not reporting all categories of Elasticsearch error

Closes: elastic#94182

* move validation to server side

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
alexwizp added a commit that referenced this pull request Jun 30, 2021
#102926) (#103840)

* [TSVB] Fix TSVB is not reporting all categories of Elasticsearch error

Closes: #94182

* move validation to server side

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 30, 2021
…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (178 commits)
  [test] Migrating to kbn_archiver from es_archiver - for the Maps app (elastic#103028)
  [Reporting] Reintroduce "ILM policy for managing reporting indices" (elastic#103850)
  [Security Solution][Endpoint] Allow activity log scrolling on small screens (elastic#103852)
  Allow zero (0) to unset unenroll_timeout field (elastic#103790)
  [TSVB] Metric count is depicted as `-` instead of 0 (elastic#103717)
  [Query] Es query/field base (elastic#103177)
  Remove add data button from nav (elastic#103810)
  Fix telemetry advanced setting style (elastic#103838)
  [Transform] Fix default naming and sorting fields suggestion for `top_metrics` agg (elastic#103690)
  [APM] use conventional error rate color for correlations (elastic#103500)
  Endpoint Telemetry: Agents Metrics + Policy Config / Response (elastic#102171)
  [Alerting] Fixed search results are not updated when search term is removed on Rules and Connectors page (elastic#103663)
  fix too many rernders (elastic#103672)
  [APM] Add “Analyze Data” button (elastic#103485)
  [Lens] Fix value popover spacing (elastic#103081)
  [TSVB] Fix TSVB is not reporting all categories of Elasticsearch error (elastic#102926)
  [SECURITY] Adds security links to doc link service (elastic#102676)
  Update dependency @elastic/charts to v31 (elastic#102078)
  [Security Solution][CTI] Investigation time enrichment UI (elastic#103383)
  Adds ECS guide to doc links service (elastic#102246)
  ...

# Conflicts:
#	x-pack/plugins/reporting/public/share_context_menu/register_pdf_png_reporting.tsx
@spalger spalger added the v7.15.0 label Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.14.0 v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TSVB] TSVB is not reporting all categories of Elasticsearch error Non-descriptive TSVB error: [tsvb] >
6 participants