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

Allow nested parsers on MessageProducer level (v3) #2084

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

AmmarAbouZor
Copy link
Member

@AmmarAbouZor AmmarAbouZor commented Aug 30, 2024

This PR isn't for to be merged. It's a suggestion in the scope of work on #2073 and #2078
It's inspired the suggestion in #2071

  • LogMessage doesn't implement Display anymore. Instead, it has the method try_resolve() which can provide an enum of either a text message if the message can be resolved or a template of resolved and unresolved chunks when a full resolve isn't possible + Resolve hints for the session to know which method should be provided to resovle the rest of the chunks.
  • The session has a ParseRestResolver struct which can try to resolved the rest unresolved chunks in the templates depending on the error hint, by having a SomeIP parser in the case of SomeIP Error hint.
  • If the resolver fails to parse the rest of the message, we can add the message to the faulty messages store ( When should will be implemented as part of Integrate Tracing for better Logging and Debugging  #2070 )

Here are basic benchmarks for the three PRs to solve the DLT with SomeIP case:

++++++++++++++  V1 ++++++++++++++ 

Run 1: 4051
Run 2: 4087
Run 3: 3931
Run 4: 4050
Run 5: 3989

Avg: (4051 + 4087 + 3931 + 4050 + 3989) / 5 = 4021.6


++++++++++++++  V2  ++++++++++++++

Run 1: 3852
Run 2: 3832
Run 3: 3805
Run 4: 3731
Run 5: 3788

Avg: (3852 + 3832 + 3805 + 3731 + 3788) / 5 = 3801.6


++++++++++++++  V3  ++++++++++++++

Run 1: 3917
Run 2: 4007
Run 3: 3818
Run 4: 3894
Run 5: 3875

Avg: (3917 + 4007 + 3818 + 3894 + 3875) / 5 = 3902.2

* Dependencies of DLT and SomeIP changed to Kevin forks
* Code copied in the positions form Kevin's PR except section
* Replace `Display` trait bound with `to_text()` method which can fail
* Add check for trait to explicitly tell if their parsing can fail at
  compile time.
* Reaction on these Errors still missing in processors.
* Implement Error Resolver on sessions to try parsing failed parse items
  depending on ErrorHint from the error
* Saving unresolved to a store isn't implemented.
* Add inline attribute to can_error to make sure the compiler will
  figure out this condition at compile time.
* Add more comment to the section where we are writing to a string about
  error handling + ignore the error instead of panicking.
* Clippy fixes
Set used Error parsing functions in one module to be shared in
indexer CLI too.
* Check for constant bool `CAN_ERROR` directly method without the need
  to an extra method to access it.
* Remove the unneeded inline method
@AmmarAbouZor
Copy link
Member Author

I still think this kind of solutions should be implemented only if the standard case of processing the log is with various nested parsers. Otherwise,
it would better to nest SomeIP within DLT parser to not change the general architectures to resolve a special case.

@DmitryAstafyev
Copy link
Collaborator

DmitryAstafyev commented Sep 4, 2024

@AmmarAbouZor thanks much for the possible solution. Here is a short summary by all current variants

Criteria V.1 (current state) V.2 V.3
Owner of nested parser initial parser Producer Producer
Scope of calling nested parser Display() of initial parser Producer Loop Producer loop
Call of nested parser During rendering Before rendering Before rendering
Nested parser error part of row (log message) bound error msg bound error msg
Render row By initial parser By initial parser By Producer with template
Changes in Producer's loop no yes yes
Changes in LogMessage trait no yes yes
  • rendering - converting to string (before write into session file)
    If I miss something, let me know please

@AmmarAbouZor
Copy link
Member Author

@AmmarAbouZor thanks much for the possible solution. Here is a short summary by all current variants

Hi @DmitryAstafyev, thanks for the useful summery for the three approaches. I think I may have a small remark on scope of V3 because this solution actually replaces Display() in the rendering phase with the new try_resolve() method, which makes calling the nested parsers rather in the rendering call than in the producer loop itself, or it sits in between.

* Resolving log messages provides an enum with text or template which
  need to be resolved.
* Sessions can try to resolve all the chunks from the templates
* Faulty messages are removed for now to keep the focus here on multiple
  parsers.
* Renaming modules, types and methods
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.

2 participants