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

CSV Upload: Translate indexing errors into plain english #7632

Closed
Bargs opened this issue Jul 5, 2016 · 9 comments
Closed

CSV Upload: Translate indexing errors into plain english #7632

Bargs opened this issue Jul 5, 2016 · 9 comments
Labels
Feature:Add Data Add Data and sample data feature on Home release_note:enhancement

Comments

@Bargs
Copy link
Contributor

Bargs commented Jul 5, 2016

CSV Upload currently returns the raw error message from Elasticsearch when there's an issue indexing a document. Based on error type, we can probably translate these errors into something more user friendly. For instance when we get a mapper_parsing_exception we could display a message like:

"Line 8: Failed to parse field @timestamp as type date"

Based on convo with @gmoskovicz in #7620

@bahaaldine
Copy link

To follow up, in the example I gave about date parsing exception: I think I've seen this error message in Elasticsearch when trying to index with Logstash, the message was something like:

"could not index field '2016-01-01 11:11:11' with date pattern 'yyyy-dd-mm hh:MM:ssSSS' ...."

So anything that we can pull out Elasticsearch response message that helps to understand why the parsing error occurred would help here.

Another point; I'm not sure we need to summarize or to filter Elasticsearch output even if it's dense, if we we switch from plain list to textarea as explained in #7648

The user will be able to have the complete log to debug; and that's always better.

@gmoskovicz
Copy link
Contributor

Agree in every point.

@tbragin
Copy link
Contributor

tbragin commented Aug 5, 2016

@Bargs One of the problems I've seen with relying on Elasticsearch errors is that when a big CSV file is uploaded, user does not see the errors until the whole file is processed. Meanwhile Elasticsearch is spewing out a stream of errors for a long time. Not sure much can be done about this until editable pipelines, but figured I'd see if you have any clever ideas.

@Bargs
Copy link
Contributor Author

Bargs commented Aug 5, 2016

@tbragin It should be fairly easy to update the UI to show indexing results in real time as they're streaming back from ES. However, I imagine the user would want to take some action based on this information, like cancel the request if they see a truck load of errors? This might be a tad bit more complicated, but I'm sure we could do it. Would it be useful at all to see realtime results without the ability to cancel a request?

@bahaaldine
Copy link

If real time logs are shown to the user, I think that a cancel action should be implemented, at least the user can stop the ingestion process and if some data has been indexed, the index should be removed.

In terms of experience, if we could simulate the indexation of a sample to see if there are potential errors, the user will know before processing the whole file what are the tweaks that he should apply, which would cause less frustration.

As far as I understood, the import data UI will at some point leverage the ingest node API and thus we could use the relative simulate API and have real time logs there.

@Bargs
Copy link
Contributor Author

Bargs commented Aug 5, 2016

As far as I understood, the import data UI will at some point leverage the ingest node API and thus we could use the relative simulate API and have real time logs there.

This is true, however the pipeline simulate functionality doesn't take existing mappings into account so it won't catch those errors, and I suspect mapping errors are the most common.

@tbragin
Copy link
Contributor

tbragin commented Aug 5, 2016

@Bargs That's a good point... probably not. Elasticsearch has a task management API, would that be something that could be used to track the progress of and cancel a request? https://www.elastic.co/guide/en/elasticsearch/reference/current/tasks.html

@Bargs
Copy link
Contributor Author

Bargs commented Aug 5, 2016

We'd have to implement it on the Kibana side, the CSV actually gets ingested in mini 100 doc bulk requests, so we'd need a way to tell Kibana to stop sending those bulk requests. Now that I think about it, it might be enough to simply kill the request from the client side, because Kibana sends those bulk requests as it receives the data from the client in a stream. I'll have to play around with it, might turn out to be easier than I thought.

@tbragin tbragin added the Feature:Add Data Add Data and sample data feature on Home label Sep 16, 2016
@epixa
Copy link
Contributor

epixa commented Dec 26, 2016

I'm going to close this since CSV upload was pulled. We can always refer back to this issue if/when we revisit the feature.

@epixa epixa closed this as completed Dec 26, 2016
mistic pushed a commit that referenced this issue Apr 18, 2024
`v93.6.0` ⏩ `v94.1.0`

> [!important]
> 👋 Hello everyone - this is a special release containing `EuiTable`'s
conversion to Emotion, several long-overdue cleanups and breaking
changes, and one or two fun new features. First, let's address the big
questions:

### Q: I'm listed as a codeowner, how much should I manually QA/review?

Answer: It depends on what exactly in your code changed, but _in
general_ I would strongly suggest at least pulling this branch down and
doing a quick visual smoke test of all tables (_note: **not**
datagrids_) in your apps/plugins. You should not expect to see any major
visual regressions.

If your table contained any kind of custom styling or behavior (e.g.
custom CSS, etc.) I **strongly** recommend taking more time to QA
thoroughly to ensure your table still looks and behaves as expected.
Teams with very manual or specific updates will be flagged below in
comment threads.

### Q: When do I need to review by?

This PR will be merged **after** 8.14FF. Because this upgrade touches so
many files and teams, we're aiming for asking for an admin merge by EOD
4/18 regardless of full approval status.

As always, you're welcome to ping us after merge if you find any issues
later ([see our
FAQ](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)),
as you will have until 8.15FF to catch any bugs.

### Q: What breaking changes were made, and why?

Here's a quick shortlist of all the changes made that affected the
majority of the commits in this PR:

#### <u>Removed several top-level table props</u>
- The `responsive` prop has been removed in favor of the new
`responsiveBreakpoint` prop (same `false` behavior as before)
- The following props were removed from basic and in-memory tables:
  - `hasActions`, `isSelectable`, and `isExpandable`
- These props were not used for functionality and were only used for
styling tables in mobile/responsive views, which is not a best practice
pattern we wanted for our APIs. Mobile tables are now styled correctly
without needing consumers to pass these extra props.
  - `textOnly`
- This prop was unused and had no meaningful impact on tables or table
content.

#### Removed hidden mobile vs. desktop DOM

Previously, EUI would set classes that applied `display: none` CSS for
content that was hidden for mobile vs. desktop. This is no longer the
case, and content that only displays for mobile or only displays for
desktop will no longer render to the DOM at all if the table is not in
that responsive state.

This is both more performant when rendering large quantities of
cells/content, and simpler to write test assertions for when comparing
what the user actually sees vs. what the DOM ‘sees’.
(c3eeb08,
78cefcd)

#### Removed direct usages of table `className`s

EuiTable `classNames` no longer have any styles attached to them, so
some instances where Kibana usages were applying the `className` for
styles have been replaced with direct component usage or removed
entirely (86ce80b).

#### Custom table cell styles

Any custom CSS for table cells was previously being applied to the inner
`div.euiTableCellContent` wrapper. As of this latest release, the
`className` and `css` props will now be applied directly to the outer
`td.euiTableRowCell` element. If you were targeting custom styles table
cells, please re-QA your styles to ensure everything still looks as
expected.

---

<details open><summary>Full changelog (click to collapse)</summary>

##
[`v94.1.0-backport.0`](https://github.com/elastic/eui/releases/v94.1.0-backport.0)

**This is a backport release only intended for use by Kibana.**

**Bug fixes**

- Fixed a visual text alignment regression in `EuiTableRowCell`s with
the `row` header scope
([#7681](elastic/eui#7681))

**Accessibility**

- Improved `EuiBasicTable` and `EuiInMemoryTable`'s selection checkboxes
to have unique aria-labels per row
([#7672](elastic/eui#7672))

## [`v94.1.0`](https://github.com/elastic/eui/releases/v94.1.0)

- Updated `EuiTableHeaderCell` to show a subdued `sortable` icon for
columns that are not currently sorted but can be
([#7656](elastic/eui#7656))
- Updated `EuiBasicTable` and `EuiInMemoryTable`'s
`columns[].actions[]`'s to pass back click events to `onClick` callbacks
as the second callback
([#7667](elastic/eui#7667))

## [`v94.0.0`](https://github.com/elastic/eui/releases/v94.0.0)

- Updated `EuiTable`, `EuiBasicTable`, and `EuiInMemoryTable` with a new
`responsiveBreakpoint` prop, which allows customizing the point at which
the table collapses into a mobile-friendly view with cards
([#7625](elastic/eui#7625))
- Updated `EuiProvider`'s `componentDefaults` prop to allow configuring
`EuiTable.responsiveBreakpoint`
([#7625](elastic/eui#7625))

**Bug fixes**

- `EuiBasicTable` & `EuiInMemoryTable` `isPrimary` actions are now
correctly shown on mobile views
([#7640](elastic/eui#7640))
- Table `mobileOptions`:
([#7642](elastic/eui#7642))
- `mobileOptions.align` is now respected instead of all cells being
forced to left alignment
- `textTruncate` and `textOnly` are now respected even if a `render`
function is not passed

**Breaking changes**

- Removed unused `EuiTableHeaderButton` component
([#7621](elastic/eui#7621))
- Removed the `responsive` prop from `EuiTable`, `EuiBasicTable`, and
`EuiInMemoryTable`. Use the new `responsiveBreakpoint` prop instead
([#7625](elastic/eui#7625))
- The following props are no longer needed by `EuiBasicTable` or
`EuiInMemoryTable` for responsive table behavior to work correctly, and
can be removed: ([#7632](elastic/eui#7632))
  - `isSelectable`
  - `isExpandable`
  - `hasActions`
- Removed the `showOnHover` prop from `EuiTableRowCell` /
`EuiBasicTable`/`EuiInMemoryTable`'s `columns` API. Use the new actions
`columns[].actions[].showOnHover` API instead.
([#7640](elastic/eui#7640))
- Removed top-level `textOnly` prop from `EuiBasicTable` and
`EuiInMemoryTable`. Use `columns[].textOnly` instead.
([#7642](elastic/eui#7642))

**DOM changes**

- `EuiTable` mobile headers no longer render in the DOM when not visible
(previously rendered with `display: none`). This may affect DOM testing
assertions. ([#7625](elastic/eui#7625))
- `EuiTableRowCell` now applies passed `className`s to the parent `<td>`
element, instead of to the inner cell content `<div>`.
([#7631](elastic/eui#7631))
- `EuiTableRow`s rendered by basic and memory tables now only render a
`.euiTableRow-isSelectable` className if the selection checkbox is not
disabled ([#7632](elastic/eui#7632))
- `EuiTableRowCell`s with `textOnly` set to `false` will no longer
attempt to apply the `.euiTableCellContent__text` className to child
elements. ([#7641](elastic/eui#7641))
- `EuiTableRowCell` no longer renders mobile headers to the DOM unless
the current table is displaying its responsive view.
([#7642](elastic/eui#7642))
- `EuiTableHeaderCell` and `EuiTableRowCell` will no longer render in
the DOM at all on mobile if their columns' `mobileOptions.show` is set
to `false`. ([#7642](elastic/eui#7642))
- `EuiTableHeaderCell` and `EuiTableRowCell` will no longer render in
the DOM at all on desktop if their columns' `mobileOptions.only` is set
to `true`. ([#7642](elastic/eui#7642))

**CSS-in-JS conversions**

- Converted `EuiTable`, `EuiTableRow`, `EuiTableRowCell`, and all other
table subcomponents to Emotion
([#7654](elastic/eui#7654))
- Removed the following `EuiTable` Sass variables:
([#7654](elastic/eui#7654))
  - `$euiTableCellContentPadding`
  - `$euiTableCellContentPaddingCompressed`
  - `$euiTableCellCheckboxWidth`
  - `$euiTableHoverColor`
  - `$euiTableSelectedColor`
  - `$euiTableHoverSelectedColor`
  - `$euiTableActionsBorderColor`
  - `$euiTableHoverClickableColor`
  - `$euiTableFocusClickableColor`
- Removed the following `EuiTable` Sass mixins:
([#7654](elastic/eui#7654))
  - `euiTableActionsBackgroundMobile`
  - `euiTableCellCheckbox`
  - `euiTableCell`
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Add Data Add Data and sample data feature on Home release_note:enhancement
Projects
None yet
Development

No branches or pull requests

5 participants