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

feat: capture caused_by and return it as part of BulkIndexerResponseItem #167

Closed
wants to merge 3 commits into from

Conversation

kyungeunni
Copy link
Contributor

@kyungeunni kyungeunni commented May 3, 2024

closes #166

Parse caused_by from the response, and add it to BulkIndexerResponseItem so that it can be logged.

TODO

  • Decide whether to make it optional (verbose:on/off) : having it by default sounds reasonable as it's an important bit of information

@kyungeunni kyungeunni requested a review from a team as a code owner May 3, 2024 10:37
@elastic-apm-tech elastic-apm-tech added the safe-to-test Automated label for running bench-diff on forked PRs label May 3, 2024
@axw
Copy link
Member

axw commented May 6, 2024

Decide whether to make it optional (verbose:on/off)

This bit is critical. Do you have a plan for how we are going decide this?
I think the PR should probably be in draft until this is decided.

@kyungeunni kyungeunni marked this pull request as draft May 6, 2024 23:58
@kyungeunni kyungeunni self-assigned this May 7, 2024
@kyungeunni
Copy link
Contributor Author

@axw Having discussed this with the team, it seems it'd make sense to log this by default as it would be helpful to understand the error better even though the logs become a bit verbose. Do you have any concern about this?

@kyungeunni kyungeunni marked this pull request as ready for review May 8, 2024 02:50
@axw
Copy link
Member

axw commented May 8, 2024

I'm not concerned about verbosity. My only concern is whether we might leak document details in the caused_by. Do we know that that won't happen?

@kyungeunni
Copy link
Contributor Author

The decision on this was to make it work similar to #165 (log it when opting in) as we are not 100% certain about caused_by not leaking any document details. Closing this for now, and will revisit it when we are happy to go ahead with #165.

@kyungeunni kyungeunni closed this May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe-to-test Automated label for running bench-diff on forked PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Capture "caused_by" in errors
3 participants