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

Better names and types for ingest stats #93533

Merged
merged 6 commits into from
Feb 7, 2023

Conversation

joegallo
Copy link
Contributor

@joegallo joegallo commented Feb 6, 2023

Closes #80763

Follow up to #93483

Adds a specialized internal OnFailureProcessor wrapper that formalizes on_failure and ignore_failure handling, and then adds unwrapping of that class when we calculate stats (similar to how ConditionalProcessor is already unwrapped). The result is greatly improved clarity and consistency for the names and types that we report for ingest stats.

@joegallo joegallo added >bug :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP Team:Data Management Meta label for data/management team v8.7.0 labels Feb 6, 2023
@joegallo joegallo requested a review from masseyke February 6, 2023 19:52
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@elasticsearchmachine
Copy link
Collaborator

Hi @joegallo, I've created a changelog YAML for you.


static final String TYPE = "on_failure";

public OnFailureProcessor(boolean ignoreFailure, Processor processor, List<Processor> onFailureProcessors) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have any test coverage (in OnFailureProcessorTests or IngestStatsNamesAndTypesIT) for when onFailureProcessors is not empty, do we? Might also be worth documenting that that is non-empty if there is an on_failure block, and empty if ignore_failure = true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pulled this out into its own PR, see #93573.

Copy link
Member

@masseyke masseyke left a comment

Choose a reason for hiding this comment

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

As discussed offline, there are countless things that this PR doesn't fix (and it's not trying to fix them), but it's definitely better than what we have right now.

@joegallo joegallo merged commit 815e6d7 into elastic:main Feb 7, 2023
@joegallo joegallo deleted the ingest-stats-names-and-types branch February 7, 2023 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP Team:Data Management Meta label for data/management team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nodes/stats for ingest pipeline name and type may be misleading
3 participants