otp icon indicating copy to clipboard operation
otp copied to clipboard

Improve observer nodes list menu

Open gomoripeti opened this issue 2 years ago • 10 comments

  • Also include connected, hidden nodes (Observer User Guide uses a hidden node as example)
  • Always include current node. If dist_listen is false it won't show up in epdm names
  • If the default epmd module erl_epmd is used and erl_epmd_port is set to DistPort then it is only possible to connect to the node listening on DistPort (if any), so exclude other nodes registered in EPMD

The last change is controversial, maybe it should be implemented elsewhere (in net_adm or erl_epmd?) and it's not the responsibility of observer to know about this detail. Or the exclusion should not happen at all (Observer shows a nice error dialogue when connection fails and maybe that's good enough)

gomoripeti avatar May 30 '22 00:05 gomoripeti

CT Test Results

    2 files    15 suites   7m 6s :stopwatch: 154 tests 152 :heavy_check_mark: 2 :zzz: 0 :x: 170 runs  168 :heavy_check_mark: 2 :zzz: 0 :x:

Results for commit e26584bc.

:recycle: This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

github-actions[bot] avatar May 30 '22 00:05 github-actions[bot]

Hidden nodes where not include for erl_interface nodes and so on, nodes that can't handle all rpc:call that are done to fetch data, so I don't think I like that. Then observer could crash c/java nodes that gets request it can't handle.

dgud avatar Jun 08 '22 13:06 dgud

thanks for the feedback, I did not think of non-erlang nodes.

The primary issue that I wanted to address is if the local node is a hidden node then all its connections will be hidden so no remote node will be listed, even if the remote nodes themselves are not hidden and fully support observer functionality.

Could there be some check only for connected nodes if they support observer functionality and only list those that do?

Do c/java nodes show up in epmd names? (If yes, they would also be listed without being connected)

gomoripeti avatar Jun 08 '22 13:06 gomoripeti

Could there be some check only for connected nodes if they support observer functionality and only list those that do?

I don't know how to test that.

Do c/java nodes show up in epmd names? (If yes, they would also be listed without being connected)

They can do that but then they expose themselves to maybe get rpc:call's so I think that is ok.

dgud avatar Jun 10 '22 11:06 dgud

A connected node is listed among nodes(hidden) if either the local node or the remote node is hidden. Would it be an acceptable heuristic that if the current node is hidden then entries from nodes(hidden) are listed? (There is an edge case when a c/java node is connected to a hidden node - but I dont know how likely that is)

gomoripeti avatar Jun 12 '22 13:06 gomoripeti

After (not) sleeping on this for a couple of nights, I I think I like your last suggestion :-)

dgud avatar Jun 16 '22 12:06 dgud

happy to hear that, I will update the PR accordingly wish you some sleep-full nights :)

gomoripeti avatar Jun 16 '22 12:06 gomoripeti

there are some recent changes in net_kernel around hidden type which I made use of. Naming is bit inconsistent with connection_type and publish_type having the same values (but bit different use)

gomoripeti avatar Jun 17 '22 23:06 gomoripeti

So that didn't go well, the erts team didn't like this idea at all.

They think we should work towards the correct way and add a capability on the erlang nodes that will tell if they are OTP nodes or not.

So sorry about fooling you, and let's go back to your original proposal with hidden nodes shown until that capability are implemented.

It would be great if you can add help field in the #create_menu{} record (in get_nodes()) with a warning about connecting to nodes that are not erlang nodes may crash them, that should make it appear if the user hover the mouse of the menu item.

dgud avatar Jun 20 '22 14:06 dgud

ok, I will add the warning text (had to try to know it was a bad idea)

gomoripeti avatar Jun 24 '22 11:06 gomoripeti