-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
Fix/domain order #114
Fix/domain order #114
Conversation
…rter' into feature/hubblo-org#75-shortcomings-of-current-json-exporter
…-shortcomings-of-current-json-exporter Feature/hubblo-org#75 shortcomings of current json exporter
For all sockets in a topology.
Use a field, populated when adding a new socket. This avoid rebuilding the list everytime we query it.
using the precomputed sorted domains list
No need to rely on a array of names, the Domain object already has a name field.
I've just added a fix for the json exporter, it was actually much simpler, we just needed to use the name field from the domain. |
I can confirm that this works on my computer, both for stdout and json exporters 👍 |
Great news, thanks for the feedback @wanecek ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me. Thanks a lot for that PR !
Before merging, I'm wondering why the 'package' domain is not handled as the other domains ? Is there any reason I'm missing ? I could try to change that but it would probably have an impact on all other exporters.
Do you mean why it was not contained in the static list you got rid of ?
For bad reasons I think, pkg is a domain that is seen on pretty new hardware afaik. I didn't get an occasion to get it in my tests yet.
I also think PR #97 is not needed anymore thanks to this one. Am I right or did I forget something ?
This should fix #108 and ensure that all detected domains are displayed. Let me know what you think.
Before merging, I'm wondering why the 'package' domain is not handled as the other domains ? Is there any reason I'm missing ? I could try to change that but it would probably have an impact on all other exporters.
Besides, I have yet to check the Json exporter to check if the same fix is needed.