-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Discover] Show ignored field values #115040
Conversation
…/kibana into dataview/flatten-hit-deprecation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
░░░░░░░░░░░████░░░░░░░░░░░░░░░░░░░░
░░░░░░░░░███░██░░░░░░░░░░░░░░░░░░░░
░░░░░░░░░██░░░█░░░░░░░░░░░░░░░░░░░░
░░░░░░░░░██░░░██░░░░░░░░░░░░░░░░░░░
░░░░░░░░░░██░░░███░░░░░░░░░░░░░░░░░
░░░░░░░░░░░██░░░░██░░░░░░░░░░░░░░░░
░░░░░░░░░░░██░░░░░███░░░░░░░░░░░░░░
░░░░░░░░░░░░██░░░░░░██░░░░░░░░░░░░░
░░░░░░░███████░░░░░░░██░░░░░░░░░░░░
░░░░█████░░░░░░░░░░░░░░███░██░░░░░░
░░░██░░░░░████░░░░░░░░░░██████░░░░░
░░░██░░████░░███░░░░░░░░░░░░░██░░░░
░░░██░░░░░░░░███░░░░░░░░░░░░░██░░░░
░░░░██████████░███░░░░░░░░░░░██░░░░
░░░░██░░░░░░░░████░░░░░░░░░░░██░░░░
░░░░███████████░░██░░░░░░░░░░██░░░░
░░░░░░██░░░░░░░████░░░░░██████░░░░░
░░░░░░██████████░██░░░░███░██░░░░░░
░░░░░░░░░██░░░░░████░███░░░░░░░░░░░
░░░░░░░░░█████████████░░░░░░░░░░░░░
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
src/plugins/discover/public/application/apps/main/components/layout/discover_documents.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reporting changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so far it's looking pretty good, still trying to find the edge case that nobody ever thought of 🔎
src/plugins/discover/public/application/components/table/table_row_btn_filter_add.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/components/table/table_row_btn_filter_remove.tsx
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
@@ -109,6 +115,28 @@ export function flattenHit( | |||
flatten(hit.fields || {}); | |||
if (params?.source !== false && hit._source) { | |||
flatten(hit._source as Record<string, any>); | |||
} else if (params?.includeIgnoredValues && hit.ignored_field_values) { | |||
// If enabled merge the ignored_field_values into the flattened hit. This will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe I am missing something, but shouldn't there be a check if fields are being read from fields API then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignored_field_values
will only be set if requesting the fields
API, and thus we're treating this here the same as the rest of that flatten logic: we ignore the actual request and purely look at the response to flatten, e.g. we also merge fields
above ignoring what the request might have been, and simply try to merge it if it's available.
The only case here (as stated by the comment) is, that there might be theoretically _source and fields and ignored_field_values, in which case we don't want to merge the ignored_field_values
, since all those values were already in _source and thus we're only doing this logic if we didn't merge _source
above.
Object.entries(hit.ignored_field_values).forEach(([fieldName, fieldValue]) => { | ||
if (flat[fieldName]) { | ||
// If there was already a value from the fields API, make sure we're merging both together | ||
if (Array.isArray(flat[fieldName])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think this would be more readable if it was all one if-statement
:
const valueExists = !!flat[fieldName]
if (valueExists && Array.isArray(flat[fieldName]))
....
else if (valueExists)
...
else
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mixed feelings around that. I feel making the 2nd else
basically dependant on the first (in the sense, that the first one is not hit and then recheck half of the condition from the first), imho does not increase readability. We also don't do this in other places, e.g. https://github.com/timroes/kibana/blob/ignored-field-values/src/plugins/discover/public/application/helpers/format_hit.ts#L55-L61 which could also be transformed the same way into:
if (displayKey && fieldsToShow.includes(key)) {
pairs.push([displayKey, formattedValue]);
} else if (!displayKey) {
pairs.push([key, formattedValue]);
}
So maybe we can leave that to a democratic vote :)
🚀 - Flatten this out into one dimension as suggested above
🎉 - Keep it in the current nested way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"I don't think it would be more readable the way you suggest, Maja, I'd like to keep it as it is" would have been a perfectly fine response
@@ -141,10 +152,9 @@ export const TableRow = ({ | |||
); | |||
} else { | |||
columns.forEach(function (column: string) { | |||
// when useNewFieldsApi is true, addressing to the fields property is safe | |||
if (useNewFieldsApi && !mapping(column) && !row.fields![column]) { | |||
if (useNewFieldsApi && !mapping(column) && row.fields && !row.fields[column]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaik, fields API should never not return at least an empty object. maybe worth checking with the ES team to see if this scenario is intentional?
* will be merged into the flattened document. This will only have an effect if | ||
* the `hit` has been retrieved using the `fields` option. | ||
*/ | ||
includeIgnoredValues?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason we don't make this default to true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've talked a bit in the PR description about this. I believe that the behavior of merging a response from the fields
API with ignored values together might be more of an edge-case that we need in Discover (where users are more interested "in the document" than in the fields), thus I don't think we should have that on by default. Those values coming from that ignored API might cause other weird side-effects potentially, so I think we should make merging them into the flattened object and opt-in imho.
case IgnoredReason.MALFORMED: | ||
return multiValue | ||
? i18n.translate('discover.docView.table.ignored.multiMalformedTooltip', { | ||
defaultMessage: `This field has one or more malformed values that can't be searched or filtered.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: for consistency, can this message be "One or more values in this field...", same as for the other two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @gchaps I took your recommendations here, so you might want to give your opinion on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either format works. I went with the text "This field has one or more malformed values that can't be searched or filtered.`" because it is shorter.
jest.mock('../../../../../kibana_services', () => ({ | ||
...jest.requireActual('../../../../../kibana_services'), | ||
getServices: () => jest.requireActual('../../../../../__mocks__/services').discoverServiceMock, | ||
})); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should refactor the mock to something like getServicesMock
, that would also allow you to configure it, if needed ... but not in this PR .. just loud thinking ..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, tested locally and a-la-carte with Chrome, Safari, Firefox works as expected, didn't manage to break it, great work of simplifying and documenting, and overall restoring behavior of Discover the way it should be! 🥳
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested locally in Chrome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this quite extensively in Chrome on Mac OS X, looks good.
💛 Build succeeded, but was flaky
Metrics [docs]Module Count
Public APIs missing comments
Any counts in public APIs
Async chunks
Page load bundle
Unknown metric groupsAPI count
References to deprecated APIs
History
To update your PR or re-run it, just comment with: |
💔 Backport failed
To backport manually run: |
…-link-to-kibana-app * 'master' of github.com:elastic/kibana: (30 commits) Fix potential error from undefined (elastic#115562) [App Search, Crawler] Fix validation step panel padding/whitespace (elastic#115542) [Cases][Connectors] ServiceNow ITOM: MVP (elastic#114125) Change default session idle timeout to 8 hours. (elastic#115565) Upgrade EUI to v39.1.1 (elastic#114732) [App Search] Wired up organic results on Curation Suggestions view (elastic#114717) [i18n] remove i18n html extractor (elastic#115004) [Logs/Metrics UI] Add deprecated field configuration to Deprecations API (elastic#115103) [Transform] Add alerting rules management to Transform UI (elastic#115363) Update UI links to Fleet and Agent docs (elastic#115295) [ML] Adding ability to change data view in advanced job wizard (elastic#115191) Change deleteByNamespace to include legacy URL aliases (elastic#115459) [Unified Integrations] Remove and cleanup add data views (elastic#115424) [Discover] Show ignored field values (elastic#115040) [ML] Stop reading the ml.max_open_jobs node attribute (elastic#115524) [Discover] Improve doc viewer code in Discover (elastic#114759) [Security Solutions] Adds security detection rule actions as importable and exportable (elastic#115243) [Security Solution] [Platform] Migrate legacy actions whenever user interacts with the rule (elastic#115101) [Fleet] Add telemetry for integration cards (elastic#115413) 🐛 Fix single percentile case when ES is returning no buckets (elastic#115214) ... # Conflicts: # x-pack/plugins/reporting/public/management/__snapshots__/report_listing.test.tsx.snap
* WIP replacing indexPattern.flattenHit by tabify * Fix jest tests * Read metaFields from index pattern * Remove old test code * remove unnecessary changes * Remove flattenHitWrapper APIs * Fix imports * Fix missing metaFields * Add all meta fields to allowlist * Improve inline comments * Move flattenHit test to new implementation * Add deprecation comment to implementation * WIP - Show ignored field values * Disable filters in doc_table * remove redundant comments * No, it wasn't * start warning message * Enable ignored values in CSV reports * Add help tooltip * Better styling with warning plus collapsible button * Disable filtering within table for ignored values * Fix jest tests * Fix types in tests * Add more tests and documentation * Remove comment * Move dangerouslySetInnerHTML into helper method * Extract document formatting into common utility * Remove HTML source field formatter * Move formatHit to Discover * Change wording of ignored warning * Add cache for formatted hits * Remove dead type * Fix row_formatter for objects * Improve mobile layout * Fix jest tests * Fix typo * Remove additional span again * Change mock to revert test * Improve tests * More jest tests * Fix typo * Change wording * Remove dead comment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # src/plugins/data_views/public/index.ts
Summary
Fixes #101232
This PR adds support for the
ignored_field_values
API added in elastic/elasticsearch#74121. This allows now seeing values which were ignored e.g. because they were too large or too long in Discover. We track this as a bug fix, since this is how the old _source implementation in Discover worked and was often perceived as a regression bug when switching to the new fields API implementation in Discover. Those values should now show back in Discover (and CSV reporting).Ignored field values will show a warning now in Discover and filtering will be disabled on them:
Limitations:
object
as a column with belows test data). You can't run with the fields API anymore in the situation where you can add object columns, thus we consider them to only be there for backwards compatibility with old saved searches, and won't add this functionality to them.Content of this PR
ignored_field_values
in theflattenHit
implementation in tabifyDocs (the one that should be used nowadays) behind a parameter. I think the default behavior should stay ignoring these values.flattenField
instead offormatHit
(which does not support that parameter yet).JSON.stringify
(to help get rid offormatHit
).formatHit
andformatField
completely from DataViews, since Discover and the old _source field formatter were the last consumers of this.indexPattern
parameter from field formatters, since it was only used by the _source field formatter.For more details please have a look at my inline comments on this PR and the comments I added to the code.
How to test?
You will require some field values which are ignored either because they are too long or they are malformed. I've used the following commands to get different types of
ignore_above
ignored data:Console commands for inserting data
Out of scope
Out of scope of this PR (issues to be created):
Checklist
Delete any items that are not applicable to this PR.
[ ] Documentation was added for features that require explanation or tutorials[ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker listFor maintainers