ENH: collect plugin config and loading errors during data prepper launch #4816
Conversation
Signed-off-by: George Chen <qchea@amazon.com>
Signed-off-by: George Chen <qchea@amazon.com>
Signed-off-by: George Chen <qchea@amazon.com>
Signed-off-by: George Chen <qchea@amazon.com>
chenqi0805
requested review from
engechas,
graytaylor0,
dinujoh,
kkondaka,
KarstenSchnitter,
dlvenable and
oeyh
as code owners
| final StringBuilder message = new StringBuilder(); | ||
| if (pipelineName != null) { | ||
| message.append(pipelineName); | ||
| message.append(PIPELINE_DELIMITER); |
There was a problem hiding this comment.
nit: ERROR_MESSAGE_DELIMITER?
There was a problem hiding this comment.
This is between pipeline and plugin so that is why it is called pipeline DELIMITER.
| return pluginFactory.loadPlugin(Source.class, sourceSetting); | ||
| } catch (Exception e) { | ||
| final PluginError pluginError = PluginError.builder() | ||
| .pipelineName(pipelineName) |
There was a problem hiding this comment.
Should we include the "componentType?" That is, source, processor, sink, buffer`?
There was a problem hiding this comment.
Thanks for the catch! Will add that
| final StringBuilder message = new StringBuilder(); | ||
| Throwable throwable = exception; | ||
|
|
||
| while (throwable != null) { |
There was a problem hiding this comment.
We should revisit this in the future. I think ideally, the failure gives a single clear message.
There was a problem hiding this comment.
I agree. Right now some runtime exception does need going down a few layers to get the useful root cause
| .stream().filter(pluginError -> pipelineName.equals(pluginError.getPipelineName())) | ||
| .collect(Collectors.toList()); | ||
| if (!subPipelinePluginErrors.isEmpty()) { | ||
| throw new InvalidPluginConfigurationException( |
There was a problem hiding this comment.
Where is this caught and handled?
It may make sense to have a single class to take all the errors and consolidate them. Also, multiple pipelines may fail.
And is this the way we want to log it? This will likely create a single long, multi-line log. But, would it be clearer to have multiple log lines?
I'd probably throw an exception that includes a list of PluginError and then have a single class for handling errors which can then decide how to format and log these.
There was a problem hiding this comment.
It is actually caught and handled in L197
I'd probably throw an exception that includes a list of PluginError and then have a single class for handling errors which can then decide how to format and log these.
Agree this part can be further refactored
There was a problem hiding this comment.
opened a separate issue to track: #4854
Signed-off-by: George Chen <qchea@amazon.com>
Signed-off-by: George Chen <qchea@amazon.com>
Description
This PR collects all plugin validation and loading errors when launching data prepper. With this change, user can peek a more complete list of plugin errors due to plugin misconfigurations during data prepper launch.
Issues Resolved
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.