Skip to content

[Uptime] Update monitor saved object mappings#130433

Merged
shahzad31 merged 15 commits intoelastic:mainfrom
shahzad31:fix-mapping
Apr 19, 2022
Merged

[Uptime] Update monitor saved object mappings#130433
shahzad31 merged 15 commits intoelastic:mainfrom
shahzad31:fix-mapping

Conversation

@shahzad31
Copy link
Copy Markdown
Contributor

@shahzad31 shahzad31 commented Apr 17, 2022

Summary

Fixes #129424

It updates the default field name, urls, and type mapping from keyword to text, while adding .keyword fields to support sorting by each field.

This also makes importableAndExportable false, since these saved objects contains secrets and import/export without decryption doesn't makes sense.

Testing

  1. Add the following keys to Kibana.yml https://p.elstc.co/paste/-uRAA0Lw#b4xgrKoWb6yEPQgW+JitoB4WN5eTLV8hLEEaavaV4qA
  2. Navigate to Monitor Management. Click Enable Monitor Management
  3. Add at least 4 monitors, one of each type, with mock data
  4. Navigate back to Monitor Management. Ensure that no Kibana errors are present
  5. On the Monitor Management List page, click the Type column heading. Ensure that monitors are sorted by type
  6. Click the URL column heading. Ensure the monitors are sorted by url. Note: There is a known bug where sorting does not work correctly for tcp and icmp monitors. Issue is pending.
  7. Click the Name column heading. Ensure the monitors are sorted by name.
  8. Navigate to Saved Object Management. Ensure that synthetics monitors are not exportable
  9. Search within Saved Objects. Ensure that no Kibana errors are present.

name: {
type: 'keyword',
type: 'text',
fielddata: true,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

without settings this, it would raise this error

[2022-04-17T11:08:06.011+02:00][ERROR][http] ResponseError: all shards failed: search_phase_execution_exception: [illegal_argument_exception] Reason: Text fields are not optimised for operations that require per-document field data like aggregations and sorting, so these operations are disabled by default. Please use a keyword field instead. Alternatively, set fielddata=true on [synthetics-monitor.name] in order to load field data by uninverting the inverted index. Note that this can use significant memory.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we not need fielddata for other properties, do you know?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jportner any idea about this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm actually reviewing this now. I can see the error is related to the sorting feature in the monitor management list, and text fields not being optimized for sorting. This error is actually present when attempting to sort on the other fields we enable sorting for, urls and type, but does not apepar until you attempt to sort by those fields.

I am currently resolving this issue using multi fields and ensuring that sorting always uses the .keyword property. I will push directly to the branch.

type: 'keyword',
type: 'text',
},
secrets: {
Copy link
Copy Markdown
Contributor

@dominiqueclarke dominiqueclarke Apr 18, 2022

Choose a reason for hiding this comment

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

This is my fault, but can we remove this? There is no reason to have secrets indexed.

name: {
type: 'keyword',
type: 'text',
fielddata: true,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we not need fielddata for other properties, do you know?

tags: {
type: 'keyword',
type: 'text',
},
Copy link
Copy Markdown
Contributor

@dominiqueclarke dominiqueclarke Apr 18, 2022

Choose a reason for hiding this comment

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

We might want to review upcoming feature requests for searching, and ensure that anything we want to search in the future has mappings.

One thing that comes to mind, for example, is location name.

That being said, we may end up simply converting Uptime saved objects to Synthetics saved objects in the future, and Uptime may never get full search and filter features

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We might want to review upcoming feature requests for searching, and ensure that anything we want to search in the future has mappings.

You can update the mappings after the fact.
For this release, I'd recommend omitting all fields that don't need to be searched in this release.

See https://docs.elastic.dev/kibana-dev-docs/tutorials/saved-objects#mappings for more details.

@dominiqueclarke
Copy link
Copy Markdown
Contributor

@elasticmachine merge upstream

@shahzad31 shahzad31 marked this pull request as ready for review April 18, 2022 10:12
@shahzad31 shahzad31 requested a review from a team as a code owner April 18, 2022 10:12
@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Apr 18, 2022
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/uptime (Team:uptime)

@dominiqueclarke
Copy link
Copy Markdown
Contributor

@elasticmachine merge upstream

@dominiqueclarke dominiqueclarke added the bug Fixes for quality problems that affect the customer experience label Apr 18, 2022
@justinkambic
Copy link
Copy Markdown
Contributor

I noted this offline to @dominiqueclarke but aside from the sort not working between individual http/browser monitors, everything seems to fall within the parameters expected by her instructions.

Here's a GIF of me "sorting" the monitors:

20220418165233

When I added another http monitor, the URLs did sort based on my input.

@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
uptime 777.2KB 777.2KB +83.0B

History

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

@shahzad31
Copy link
Copy Markdown
Contributor Author

Everything seems to be working as expected including sorting.

Copy link
Copy Markdown
Contributor

@lucasfcosta lucasfcosta left a comment

Choose a reason for hiding this comment

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

I have reviewed the code as per @shahzad31's request (after reading this super helpful explanation) and it LGTM.

I haven't E2E this PR as per Shahzad's request considering this PR has already been tested.

@shahzad31 shahzad31 merged commit a843db3 into elastic:main Apr 19, 2022
@shahzad31 shahzad31 deleted the fix-mapping branch April 19, 2022 08:19
@shahzad31 shahzad31 added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 19, 2022
@kibanamachine
Copy link
Copy Markdown
Contributor

💔 All backports failed

Status Branch Result
8.2 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 130433

Questions ?

Please refer to the Backport tool documentation

shahzad31 added a commit to shahzad31/kibana that referenced this pull request Apr 19, 2022
* update mappings

* PR feedback

* update

* update mappings

* update mappings

* port and hosts

* adjust jest test

* adjust mappings for sorting by url, name, and type

* remove unused import

* synthetics - do not make synthetics monitors exportable

* adjust tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Dominique Clarke <dominique.clarke@elastic.co>
(cherry picked from commit a843db3)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 19, 2022
…disable-server-side

* 'main' of github.com:elastic/kibana: (103 commits)
  [Osquery] Update eslint config (elastic#129637)
  [Uptime] Update monitor saved object mappings (elastic#130433)
  Add links to metricbeat module docs (elastic#130519)
  Add link to troubleshooting guide in confirm data copy (elastic#130420)
  [Step 3] Cleanup charts plugin (elastic#130132)
  [Visualize] Adds a deprecation warning to the pie app (elastic#130447)
  [Maps] fix vector tile load errors not displayed in legend (elastic#130395)
  [CI] Split alerting-api-integration tests into separate cigroups (elastic#130414)
  [CI] Use spot instances for default cigroups in PR CI (elastic#130476)
  [functional-tests] TimePicker optimizations (elastic#130200)
  [kbn/pm] use stable module ids in dist (elastic#130497)
  [8.2.1][Security Solution][Session view] fix full screen session view margin (elastic#130496)
  Fix wrong config in comments (elastic#130378)
  Add deprecated telemetry (elastic#130458)
  Add eslint rule to support breaking up packages (elastic#130483)
  [Security Solution][Endpoint] Fix test stability and un-skip flaky tests (elastic#130176)
  Update object types for SharePoint Online external connector (elastic#130478)
  [Workplace Search] Fix broken feedback link (elastic#130475)
  Rename the term "execution" in config to "run" (elastic#130172)
  [Cloud Posture] use index with keyword mapping (elastic#130456)
  ...

# Conflicts:
#	docs/user/reporting/index.asciidoc
#	x-pack/plugins/reporting/public/types.ts
#	x-pack/plugins/screenshotting/server/screenshots/index.test.ts
#	x-pack/plugins/screenshotting/server/screenshots/index.ts
shahzad31 added a commit that referenced this pull request Apr 19, 2022
* update mappings

* PR feedback

* update

* update mappings

* update mappings

* port and hosts

* adjust jest test

* adjust mappings for sorting by url, name, and type

* remove unused import

* synthetics - do not make synthetics monitors exportable

* adjust tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Dominique Clarke <dominique.clarke@elastic.co>
(cherry picked from commit a843db3)
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
* update mappings

* PR feedback

* update

* update mappings

* update mappings

* port and hosts

* adjust jest test

* adjust mappings for sorting by url, name, and type

* remove unused import

* synthetics - do not make synthetics monitors exportable

* adjust tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Dominique Clarke <dominique.clarke@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.2.0 v8.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8.2.0 BC1: "Bad Request" error when searching saved objects

8 participants