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

[Framework] Allow bulk operations result logging #2117

Merged
merged 21 commits into from
Mar 27, 2024

Conversation

timgrein
Copy link
Contributor

@timgrein timgrein commented Feb 5, 2024

We've an open tech improvement, which states that it's important for some customers for audit purposes to be able to log ids of documents, which were deleted.

This PR introduces one new configuration setting elasticsearch.bulk.enable_operations_logging to enable this option. The logs will be logged on DEBUG level.

Sample logs for successful operations:

[...]
[FMWK][12:02:59][DEBUG] [Connector id: PHx4ZY4B3uUGqDXsM_KG, index name: mysql-connector, Sync job id: QnzTZY4B3uUGqDXs5PJS] Successfully executed 'index' on document with id 'employees_1702'. Result: created
[FMWK][12:02:59][DEBUG] [Connector id: PHx4ZY4B3uUGqDXsM_KG, index name: mysql-connector, Sync job id: QnzTZY4B3uUGqDXs5PJS] Successfully executed 'delete' on document with id 'customers_112'. Result: deleted
[...]

Sample log for failed operations:

[FMWK][12:02:59][DEBUG] [Connector id: PHx4ZY4B3uUGqDXsM_KG, index name: mysql-connector, Sync job id: QnzTZY4B3uUGqDXs5PJS] Failed to execute 'delete' on document with id '123'. Result: not found. Error: Doc with id 123 was not found

Checklists

Pre-Review Checklist

  • this PR has a meaningful title
  • this PR links to all relevant github issues that it fixes or partially addresses
  • if there is no GH issue, please create it. Each PR should have a link to an issue
  • this PR has a thorough description
  • Covered the changes with automated tests
  • Tested the changes locally
  • Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)
  • Considered corresponding documentation changes
  • Contributed any configuration settings changes to the configuration reference
    • Not yet, will open a separate PR

- [ ] if you added or changed Rich Configurable Fields for a Native Connector, you made a corresponding PR in Kibana

Release Note

Add configuration settings to be able to log ids of deleted documents.

Copy link
Member

@seanstory seanstory left a comment

Choose a reason for hiding this comment

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

One nit, but largely LGTM.

@timgrein timgrein changed the title [Framework] Allow deleted doc ids to be logged depending on config [Framework] Allow tracing doc ids on create/update/delete Feb 8, 2024
Copy link
Member

@artem-shelkovnikov artem-shelkovnikov left a comment

Choose a reason for hiding this comment

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

Is it possible to do it not here, but after bulk call was made?

It should return stats of bulk request and will give you much more precise stats (e.g. when you upsert it can tell you whether it created or updated the record)

@timgrein timgrein changed the title [Framework] Allow tracing doc ids on create/update/delete [Framework] Allow tracing doc ids on create/index/delete/update Mar 22, 2024
result = item[action_item].get("result")
operation_failed = result not in SUCCESSFUL_RESULTS

if operation_failed:
Copy link
Member

Choose a reason for hiding this comment

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

For failed operation, you should check

if "error" in item[action_item]

Copy link
Contributor Author

@timgrein timgrein Mar 22, 2024

Choose a reason for hiding this comment

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

See comment above. Maybe it makes sense to name the condition non_successful_result/ failed_result?

Co-authored-by: Dmitriy Burlutskiy <dmitrii.burlutckii@elastic.co>
@timgrein timgrein requested a review from wangch079 March 22, 2024 12:41
@timgrein timgrein changed the title [Framework] Allow tracing doc ids on create/index/delete/update [Framework] Allow bulk operations result logging Mar 22, 2024
config.yml Outdated
@@ -147,6 +147,10 @@
#elasticsearch.bulk.retry_interval: 10
#
#
## Enable to log ids of created/indexed/deleted/updated documents during a sync.
Copy link
Member

Choose a reason for hiding this comment

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

help me make sure that this isn't lost with #2280 (depending on which merges first)

Copy link
Member

Choose a reason for hiding this comment

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

mine merged first. To resolve the conflict, this'll just need to be moved to config.yml.example

Copy link
Member

@seanstory seanstory left a comment

Choose a reason for hiding this comment

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

few nits. But I like it!

timgrein and others added 2 commits March 25, 2024 10:07
Co-authored-by: Sean Story <sean.j.story@gmail.com>
@timgrein timgrein requested a review from seanstory March 25, 2024 09:18
Copy link
Member

@artem-shelkovnikov artem-shelkovnikov left a comment

Choose a reason for hiding this comment

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

Good stuff!

@timgrein timgrein merged commit 0b2d390 into main Mar 27, 2024
2 checks passed
@timgrein timgrein deleted the timgrein/log-deleted-ids branch March 27, 2024 08:47
Copy link

💚 Backport PR(s) successfully created

Status Branch Result
8.13 #2301

This backport PR will be merged automatically after passing CI.

timgrein added a commit that referenced this pull request Mar 27, 2024
Co-authored-by: Tim Grein <tim.grein@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants