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

[Streams 🌊] Enrichment - Fix broken results due to condition and add skipped metric #212757

Merged

Conversation

tonyghiani
Copy link
Contributor

@tonyghiani tonyghiani commented Feb 28, 2025

📓 Summary

When the condition is not met, the processing simulation reports wrong metrics and fails on a unhandler error.

This work fix the issue and also update the document simulation metrics, reporting how many documents are skipped by a processor during the simulation.

A follow-up work will update the filters on the date to better reflect the available states of the documents (parsed, partially parsed, skipped, failed).

Screenshot 2025-02-28 at 12 47 10

@tonyghiani tonyghiani added release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-logs Observability Logs User Experience Team backport:version Backport to applied version labels Feature:Streams This is the label for the Streams Project v9.1.0 v8.19.0 labels Feb 28, 2025
@tonyghiani tonyghiani requested a review from a team as a code owner February 28, 2025 11:52
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs)

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks, great addition. Seems like the issue with dotted field names still exists on this PR, do you plan to tackle this separately?

@@ -609,6 +637,11 @@ const isSuccessfulProcessor = (
): processor is WithRequired<IngestPipelineSimulation, 'doc' | 'tag'> =>
processor.status === 'success' && !!processor.tag;

const isSkippedProcessor = (
processor: IngestPipelineSimulation
// @ts-expect-error Looks like the IngestPipelineSimulation.status is not typed correctly and misses the 'skipped' status
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it's using WatcherActionStatusOptions which seems very wrong... Could you report this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep this is wrong, I'll report that! 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tonyghiani
Copy link
Contributor Author

Seems like the issue with dotted field names still exists on this PR, do you plan to tackle this separately?

I'll look into this today, if I see it's a simple change I'll probably fix it here and ping you for a final check

@tonyghiani
Copy link
Contributor Author

@flash1293 I added the changes to handle nested fields in the condition and seems to work well, this is ready for another check 👌

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Accessing dotted fields works, but why the change of including skipped into the failure rate? That seems wrong to me

@tonyghiani
Copy link
Contributor Author

Accessing dotted fields works, but why the change of including skipped into the failure rate? That seems wrong to me.

Yeah I did it cause we introduces the new status of "skipped" document, and this wasn't accounted for anywhere, so I added there to at least put it in a team. I already discussed with Luca offline about this, and we might want to improve the filters on the table to match all of them: parsed, partially parsed, skipped, failed. It's a temporary change to get the matched/unmatched rates compensate each other correctly.

@flash1293
Copy link
Contributor

But you just changed this in the last commit, right? 950fca0

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Anyway, I think it's fine - let's merge like this and see whether people understand.

Definitely like calling out skipped separately from failed with the badges.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
streamsApp 307.3KB 307.6KB +353.0B

History

@tonyghiani tonyghiani merged commit 6e2a103 into elastic:main Mar 4, 2025
9 checks passed
@tonyghiani tonyghiani deleted the fix/stream-simulation-skipped-processors branch March 4, 2025 06:43
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/13647841963

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 4, 2025
…skipped metric (elastic#212757)

## 📓 Summary

When the condition is not met, the processing simulation reports wrong
metrics and fails on a unhandler error.

This work fix the issue and also update the document simulation metrics,
reporting how many documents are skipped by a processor during the
simulation.

A follow-up work will update the filters on the date to better reflect
the available states of the documents (parsed, partially parsed,
skipped, failed).

<img width="701" alt="Screenshot 2025-02-28 at 12 47 10"
src="https://github.com/user-attachments/assets/1b6979e4-78a1-4db3-af72-faaf06c0e249"
/>

(cherry picked from commit 6e2a103)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels Feature:Streams This is the label for the Streams Project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-logs Observability Logs User Experience Team v8.19.0 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants