Skip to content

Conversation

@ruflin
Copy link
Contributor

@ruflin ruflin commented Dec 20, 2023

There are two fixes in this PR. The first one is around the runner being stuck in case of errors. Because a channel is used for the errors, in case 2 errors happen in a row before the error is consumed, the go routine gets stuck. As the effect of an error is stopping the generation anyways, now on error the progress on function is aborted with return. As the errors are mostly used for logging, it raises the question if the channels are needed in the first place but further cleanup is out of scope of this PR.

The second change is around error logging for scenarios where the template generates invalid JSON. For easier debugging, it now prints out the full invalid JSON event as a log event and gives an indication on where the problem lies.

There are two fixes in this PR. The first one is around the runner being stuck in case of errors. Because a channel is used for the errors, in case 2 errors happen in a row before the error is consumed, the go routine gets stuck. As the effect of an error is stopping the generation anyways, now on error the progress on function is aborted with return. As the errors are mostly used for logging, it raises the question if the channels are needed in the first place but further cleanup is out of scope of this PR.

The second change is around error logging for scenarios where the template generates invalid JSON. For easier debugging, it now prints out the full invalid JSON event as a log event and gives an indication on where the problem lies.
@ruflin ruflin self-assigned this Dec 20, 2023
@ruflin ruflin requested a review from jsoriano December 20, 2023 15:45
Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
@ruflin
Copy link
Contributor Author

ruflin commented Dec 21, 2023

Thanks @jsoriano for the proposal. Lets get this in as is for now. The only downside is that now a dev has to enable debug logging during development. But in the end, the problem should not be solved here but with #1610

@jsoriano
Copy link
Member

/test

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Thanks!

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @ruflin

@ruflin ruflin merged commit 56e1a4f into elastic:main Dec 21, 2023
@ruflin ruflin deleted the fix-shutdown-and-error branch December 21, 2023 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants