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

Refactoring of introspection #518

Open
1 task
FerdinandSpitzschnueffler opened this issue Jan 22, 2021 · 7 comments
Open
1 task

Refactoring of introspection #518

FerdinandSpitzschnueffler opened this issue Jan 22, 2021 · 7 comments
Labels
good first issue Good for newcomers refactoring Refactor code without adding features

Comments

@FerdinandSpitzschnueffler
Copy link
Contributor

FerdinandSpitzschnueffler commented Jan 22, 2021

Brief feature description

The introspection needs a refactoring regarding:

  • how is data internally stored and accessible for introspection
  • how is this data sent to the client
  • which information should be presented and how
  • introspection overhead should be minimal when the introspection is not used
  • consider reserving the service string iceoryx for internal publishers only

ToDo:

  • Fix/Design or refactor error handling in ProcessIntrospection::run()

Detailed information

When the introspection was implemented multiple pub/sub scenarios have not been considered, e.g. when a subscriber is subscribed to multiple publishers only one connection is displayed. When one publisher stops sending data then the subscriber is shown as disconnected (while still receiving data from the other publisher). This has to be adapted.
Additionally, introspection tests are missing and the existing ones may have to be adapted once it is clear how data is internally stored and sent to the client.

Open questions:

@budrus
Copy link
Contributor

budrus commented Jan 24, 2021

When one publisher stops sending data then the subscriber is shown as disconnected (while still receiving data from the other publisher).

@FerdinandSpitzschnueffler . Is this because the intropspection processes the CaPro messages again to determine the connection state? And this "state machine" no does not fit to the mult-puhser one? I think we would have to change it in a way that state changes of subscribers are reported and not a redundant (and maybe other) processing of all CaPro messages is made

@dkroenke
Copy link
Member

Do we want to stick to ncurses?

From my point of view we can stick to ncurses for the terminal output, but we need a solution for writing the introspection data into a parseable logfile for debugging purpose.

@FerdinandSpitzschnueffler
Copy link
Contributor Author

When one publisher stops sending data then the subscriber is shown as disconnected (while still receiving data from the other publisher).

@FerdinandSpitzschnueffler . Is this because the intropspection processes the CaPro messages again to determine the connection state? And this "state machine" no does not fit to the mult-puhser one? I think we would have to change it in a way that state changes of subscribers are reported and not a redundant (and maybe other) processing of all CaPro messages is made

@budrus Yes, when one publisher stops sending data then the connection state is set to disconnected without checking whether the subscriber is also subscribed to another publisher.

@MatthiasKillat
Copy link
Contributor

MatthiasKillat commented Jan 25, 2021

We need a proper state machine to track this, and in the right place (the introspection itself is the wrong place). It also can neither be done by the subscriber or publisher alone, it requires additional information. I coined those entities connections back then with the assumption that each subscriber has at most one possible connection (to a publisher that may or may not exist yet). This assumption is not true anymore and hence a careful rework is required.

@MatthiasKillat
Copy link
Contributor

MatthiasKillat commented Jan 25, 2021

As for std::map replacement. Since the introspection does relatively frequent lookups, we need an efficient lookup mechanism, otherwise it is unacceptably slow when even a moderate number of ports are involved. I doubt a refactored lookup mechanism would work nicely without something like a map (something which provides logarithmic or even constant time lookup).

Now, creating a true map with efficent lookups with our constraints is not easy, but doable (essentially we need to implement red black trees without dynamic memory).
A temporary solution can be a map-like interface (key value pairs etc.) but an inefficient search behind this interface which can be improved upon later. Thus we would not need a full blown map right away.

Note that it is perfectly doable to implement those trees but to lift these trees as a prototype (I have done so in the past), the time consuming part is the code beautification afterwards.

Note that to my knowledge cyclonedds stores such entity information in balanced trees (AVL I guess), and probably for good reason: it admits efficient lookup, insertion and deletion. In the long term I believe we need something similar.

@elBoberido
Copy link
Member

Do we want to stick to ncurses?

From my point of view we can stick to ncurses for the terminal output, but we need a solution for writing the introspection data into a parseable logfile for debugging purpose.

I'd suggest to have a minimal introspection client dumping everything to the console and a fancy one with Python, Qt, HTML5, whatever and get rid of ncurses

@dkroenke
Copy link
Member

Additional requirements:

  • platform independent introspection
  • introspection over DDS
  • graphical interface (please make first a proposal for a GUI framework, maybe a webpage?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers refactoring Refactor code without adding features
Projects
None yet
Development

No branches or pull requests

6 participants