Add logging to importer. Closes issue #192212 #192234

kyracho · 2024-09-06T03:36:08Z

Summary

Hello, this closes issue #192212 by adding a custom logger to the importer, making debugging issues easier.

Checklist

Delete any items that are not applicable to this PR.

[N/A] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
[N/A] Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios TODO
Flaky Test Runner was used on any tests changed TODO
[N/A] Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
[N/A] Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
[N/A] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
[N/A] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
[N/A] This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately (no API changes)

cla-checker-service · 2024-09-06T03:36:12Z

💚 CLA has been signed

kyracho · 2024-09-06T03:52:00Z

Hello, I’ve now signed the contributor agreement, which covers the commit b523e16.

kyracho · 2024-09-07T19:26:56Z

Hello, I wanted to kindly request that someone with the necessary permissions add the release note label to this pull request. Unfortunately, I don't have the authority to apply labels, and it seems the label is required to pass the CI test. Thanks in advance for your assistance.

TinaHeiligers · 2024-10-03T17:00:55Z

@elasticmachine merge upstream

TinaHeiligers · 2024-10-03T18:05:59Z

@kyracho thank you for your PR! I added the required labels and updated your branch.

Adding the logger to the Saved Objects Service won't automatically instrument logging,
The logger must be passed to the SavedObjectsImporter class and used in the import and resolveImportErrors APIs. The types, unit tests, and integration tests also need updates.
See SavedObjectsExporter for how that's done.

TinaHeiligers

Adding the logger to the Saved Objects Service won't automatically instrument logging.

The logger must be passed to the SavedObjectsImporter class and used in the import and resolveImportErrors APIs. The types, unit tests, and integration tests also need updates.

TinaHeiligers · 2024-10-03T18:40:48Z

@elasticmachine merge upstream

elasticmachine · 2024-10-03T22:10:39Z

Pinging @elastic/fleet (Team:Fleet)

TinaHeiligers · 2024-10-03T22:28:51Z

@kyracho the forced pushes don't start CI. Please request us to run CI on your behalf.

TinaHeiligers · 2024-10-03T22:29:28Z

@elasticmachine merge upstream

elasticmachine · 2024-10-03T22:29:31Z

There are no new commits on the base branch.

TinaHeiligers · 2024-10-04T20:45:45Z

@kyracho you're probably facing the rebase nightmare by now. It would be a good idea to update your fork of Kibana and keep that up to date with your local clone. Don't rebase anymore, merge updates instead.
git and source control can take some getting used to, we've all been there

TinaHeiligers

import reads from a stream and awaiting the call (and other related function calls) can cause Kibana to OOM.

Logging doesn't warrant the risks of Kibana running out of memory.

TinaHeiligers · 2024-10-04T22:46:39Z

...bjects/core-saved-objects-import-export-server-internal/src/import/saved_objects_importer.ts

-    });
+    this.#log.debug('Starting the import process');
+    try {
+      const result = await importSavedObjectsFromStream({


Awaiting a resolved promise from importSavedObjectsFromStream is a problem when importing many saved objects and can cause Kibana to run out of memory.
import shouldn't await on importSavedObjectsFromStream, we have to stick with calling the method directly.

I'd rather move the log line to before returning importSavedObjectsFromStream and revert the other changes here,

See node streams for more info

TinaHeiligers · 2024-10-04T22:47:02Z

...bjects/core-saved-objects-import-export-server-internal/src/import/saved_objects_importer.ts

  }

-  public resolveImportErrors({
+  public async resolveImportErrors({


we can't 'simply' change this....

TinaHeiligers · 2024-10-04T22:47:44Z

...bjects/core-saved-objects-import-export-server-internal/src/import/saved_objects_importer.ts

-    });
+    this.#log.debug('Resolving import errors');
+    try {
+      const result = await resolveSavedObjectsImportErrors({


similar comment as for importSavedObjectsFromStream

TinaHeiligers

import reads from a stream and awaiting the call (and other related function calls) can cause Kibana to OOM.

Logging doesn't warrant the risks of Kibana running out of memory.

kyracho · 2024-10-05T00:07:31Z

@TinaHeiligers Thanks for the guidance! I’ve made the following changes in response to your comments:

Removed await from importSavedObjectsFromStream and resolveSavedObjectsImportErrors to prevent memory issues.
Moved the log statements to occur before the return statements, as you suggested
Returned the promises directly
Removed the async keywords from the methods

Let me know if anything else is needed!

TinaHeiligers · 2024-10-07T19:55:21Z

@elasticmachine merge upstream

TinaHeiligers

This PR adds simplistic logging.
We need to enhance the logs with information on the saved objects being imported, how they are imported and if there are any errors. The enhancements can be a follow up to this PR.

TinaHeiligers · 2024-10-07T21:58:14Z

buildkite test this

kyracho · 2024-10-07T22:37:14Z

Thanks, Tina! I'd be happy to work on those enhancements in a new PR if that's okay. Please let me know if you'd like me to take it on.

TinaHeiligers · 2024-10-07T23:43:56Z

buildkite test this

TinaHeiligers · 2024-10-07T23:52:24Z

Thanks, Tina! I'd be happy to work on those enhancements in a new PR if that's okay. Please let me know if you'd like me to take it on.

The idea is to use the logs for debugging, similar to how we log info during export.
Take a look at what we log for export with the following config to your kibana.dev.yml:

logging:
  appenders:
    so-console: // or use whatever you prefer
      type: console
      layout:
        type: pattern
        highlight: true
        pattern: "SOS_IMPEXP--[%date][%level][%logger]---%message"
  root:
    appenders: [console]
    level: info
  loggers:
    - name: savedobjects-service.exporter
      level: all
      appenders: [so-console] // feel free to change this
    - name: savedobjects-service.importer
      level: all
      appenders: [so-console] // feel free to change this

FYI: the appender is one I prefer to use, you can use whatever you want

Start es
Start kibana in dev (any license is fine)
Once kibana's running, add any sample data. Adding sample data uses Saved Objects import (the logs you're adding here)
Navigate to the Saved Objects Management page and export anything from there (or all of them)
Watch the terminal you started Kibana from. The logs will be there as stdout

kibanamachine · 2024-10-08T01:58:53Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11227177901

elasticmachine · 2024-10-08T01:59:10Z

💚 Build Succeeded

Buildkite Build
Commit: 30c29dc

Metrics [docs]

✅ unchanged

## Summary Hello, this closes issue elastic#192212 by adding a custom logger to the importer, making debugging issues easier. ### Checklist Delete any items that are not applicable to this PR. - [N/A] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [N/A] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios TODO - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed TODO - [N/A] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [N/A] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [N/A] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [N/A] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [N/A] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [X] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (no API changes) --------- Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit c36a894)

kibanamachine · 2024-10-08T02:03:09Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

# Backport This will backport the following commits from `main` to `8.x`: - [Add logging to importer. Closes issue #192212 (#192234)](#192234)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Kyra Cho <[email protected]>

## Summary Hello, this is a follow up PR to #192234 . The previous PR added simplistic logging to the saved objects importer. The goal now is to enhance the logs with information on the saved objects being imported, how they are imported, and by displaying any errors. #### `import_saved_objects.ts`: - Logs specific types being imported - Logs size limit and overwrite status - Logs Success/Fail messages #### Changes to `saved_objects_importer.ts`: - Passes the logger to `importSavedObjectsFromStream` - Removes "starting import" #### Changes to `import_saved_objects.test.ts`: - Updates it for the new logger parameter #### Changes to `import.test.ts`: - Uses the mock logger provided by core, instead of using a custom one ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels) - [ ] This will appear in the **Release Notes** and follow the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Alejandro Fernández Haro <[email protected]>

## Summary Hello, this is a follow up PR to elastic#192234 . The previous PR added simplistic logging to the saved objects importer. The goal now is to enhance the logs with information on the saved objects being imported, how they are imported, and by displaying any errors. #### `import_saved_objects.ts`: - Logs specific types being imported - Logs size limit and overwrite status - Logs Success/Fail messages #### Changes to `saved_objects_importer.ts`: - Passes the logger to `importSavedObjectsFromStream` - Removes "starting import" #### Changes to `import_saved_objects.test.ts`: - Updates it for the new logger parameter #### Changes to `import.test.ts`: - Uses the mock logger provided by core, instead of using a custom one ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels) - [ ] This will appear in the **Release Notes** and follow the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Alejandro Fernández Haro <[email protected]> (cherry picked from commit 45543a1)

# Backport This will backport the following commits from `main` to `8.x`: - [basic enhancements for import logging (#196056)](#196056)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Kyra Cho <[email protected]> Co-authored-by: Alejandro Fernández Haro <[email protected]>

## Summary Hello, this is a follow up PR to elastic#192234 . The previous PR added simplistic logging to the saved objects importer. The goal now is to enhance the logs with information on the saved objects being imported, how they are imported, and by displaying any errors. #### `import_saved_objects.ts`: - Logs specific types being imported - Logs size limit and overwrite status - Logs Success/Fail messages #### Changes to `saved_objects_importer.ts`: - Passes the logger to `importSavedObjectsFromStream` - Removes "starting import" #### Changes to `import_saved_objects.test.ts`: - Updates it for the new logger parameter #### Changes to `import.test.ts`: - Uses the mock logger provided by core, instead of using a custom one ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels) - [ ] This will appear in the **Release Notes** and follow the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Alejandro Fernández Haro <[email protected]>

kyracho requested a review from a team as a code owner September 6, 2024 03:36

kibanamachine added the 💝community label Sep 6, 2024

TinaHeiligers added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 3, 2024

kyracho force-pushed the add-logging-to-importer branch from ebc0ea8 to 39df481 Compare October 3, 2024 17:35

TinaHeiligers requested changes Oct 3, 2024

View reviewed changes

kyracho requested review from a team as code owners October 3, 2024 22:10

kyracho requested review from MichaelKatsoulis and constanca-m October 3, 2024 22:10

botelastic bot added ci:project-deploy-observability Create an Observability project Team:Fleet Team label for Observability Data Collection Fleet team labels Oct 3, 2024

kyracho force-pushed the add-logging-to-importer branch from b75eb81 to 61137bc Compare October 3, 2024 22:12

TinaHeiligers removed request for a team, MichaelKatsoulis and constanca-m October 3, 2024 22:31

Add logging to importer

e5111ac

TinaHeiligers requested changes Oct 4, 2024

View reviewed changes

kyracho added 3 commits October 4, 2024 16:42

Merge branch 'elastic:main' into add-logging-to-importer

6530626

change saved_objects_importer.ts

d909a11

change saved_objects_importer.ts

0c9f032

Merge branch 'main' into add-logging-to-importer

30c29dc

TinaHeiligers approved these changes Oct 7, 2024

View reviewed changes

TinaHeiligers enabled auto-merge (squash) October 7, 2024 23:52

TinaHeiligers merged commit c36a894 into elastic:main Oct 8, 2024
24 checks passed

kibanamachine added the v9.0.0 label Oct 8, 2024

kibanamachine mentioned this pull request Oct 8, 2024

[8.x] Add logging to importer. Closes issue #192212 (#192234) #195339

Merged

kyracho deleted the add-logging-to-importer branch October 11, 2024 20:49

kyracho mentioned this pull request Oct 14, 2024

basic enhancements for import logging #196056

Merged

4 tasks

afharo mentioned this pull request Oct 30, 2024

[Saved Objects] Add logging to importer #192212

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logging to importer. Closes issue #192212 #192234

Add logging to importer. Closes issue #192212 #192234

kyracho commented Sep 6, 2024 •

edited by kibanamachine

Loading

cla-checker-service bot commented Sep 6, 2024 •

edited

Loading

kyracho commented Sep 6, 2024

kyracho commented Sep 7, 2024

TinaHeiligers commented Oct 3, 2024

TinaHeiligers commented Oct 3, 2024 •

edited

Loading

TinaHeiligers left a comment

TinaHeiligers commented Oct 3, 2024

elasticmachine commented Oct 3, 2024

TinaHeiligers commented Oct 3, 2024

TinaHeiligers commented Oct 3, 2024

elasticmachine commented Oct 3, 2024

TinaHeiligers commented Oct 4, 2024 •

edited

Loading

TinaHeiligers left a comment

TinaHeiligers Oct 4, 2024

TinaHeiligers Oct 4, 2024

TinaHeiligers Oct 4, 2024

TinaHeiligers Oct 4, 2024

TinaHeiligers left a comment

kyracho commented Oct 5, 2024

TinaHeiligers commented Oct 7, 2024

TinaHeiligers left a comment

TinaHeiligers commented Oct 7, 2024

kyracho commented Oct 7, 2024

TinaHeiligers commented Oct 7, 2024

TinaHeiligers commented Oct 7, 2024

kibanamachine commented Oct 8, 2024

elasticmachine commented Oct 8, 2024

kibanamachine commented Oct 8, 2024

Add logging to importer. Closes issue #192212 #192234

Add logging to importer. Closes issue #192212 #192234

Conversation

kyracho commented Sep 6, 2024 • edited by kibanamachine Loading

Summary

Checklist

For maintainers

cla-checker-service bot commented Sep 6, 2024 • edited Loading

kyracho commented Sep 6, 2024

kyracho commented Sep 7, 2024

TinaHeiligers commented Oct 3, 2024

TinaHeiligers commented Oct 3, 2024 • edited Loading

TinaHeiligers left a comment

Choose a reason for hiding this comment

TinaHeiligers commented Oct 3, 2024

elasticmachine commented Oct 3, 2024

TinaHeiligers commented Oct 3, 2024

TinaHeiligers commented Oct 3, 2024

elasticmachine commented Oct 3, 2024

TinaHeiligers commented Oct 4, 2024 • edited Loading

TinaHeiligers left a comment

Choose a reason for hiding this comment

TinaHeiligers Oct 4, 2024

Choose a reason for hiding this comment

TinaHeiligers Oct 4, 2024

Choose a reason for hiding this comment

TinaHeiligers Oct 4, 2024

Choose a reason for hiding this comment

TinaHeiligers Oct 4, 2024

Choose a reason for hiding this comment

TinaHeiligers left a comment

Choose a reason for hiding this comment

kyracho commented Oct 5, 2024

TinaHeiligers commented Oct 7, 2024

TinaHeiligers left a comment

Choose a reason for hiding this comment

TinaHeiligers commented Oct 7, 2024

kyracho commented Oct 7, 2024

TinaHeiligers commented Oct 7, 2024

TinaHeiligers commented Oct 7, 2024

kibanamachine commented Oct 8, 2024

elasticmachine commented Oct 8, 2024

💚 Build Succeeded

Metrics [docs]

kibanamachine commented Oct 8, 2024

💚 All backports created successfully

Questions ?

kyracho commented Sep 6, 2024 •

edited by kibanamachine

Loading

cla-checker-service bot commented Sep 6, 2024 •

edited

Loading

TinaHeiligers commented Oct 3, 2024 •

edited

Loading

TinaHeiligers commented Oct 4, 2024 •

edited

Loading