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

Add more information to the Observer #1484

Merged
merged 1 commit into from
Aug 16, 2017
Merged

Add more information to the Observer #1484

merged 1 commit into from
Aug 16, 2017

Conversation

Gsantomaggio
Copy link

Add other two panels: first one: 'System limits' and second one 'Limit Statistics'
Show infomations as atom_limit,process_limit,port_limit and counts.

The result is:
observer_info

@josevalim
Copy link
Contributor

I wonder if it would make sense to group those?

System statistics / limit
-------------------------

Atoms: 11625 / 1048576 (1% used)
Processes: 45 / 262144 (0% used)
...

I am just not sure how the distribution buffer busy limit fits this. :(

@Gsantomaggio
Copy link
Author

yes, looks nicer.
about distribution buffer busy limit we can just leave the value without count and percentage

@josevalim
Copy link
Contributor

@Gsantomaggio good call. Especially if we call "Distribution buffer busy limit" and skip the word "limit" in the others.

@dgud dgud self-assigned this Jul 4, 2017
@Gsantomaggio
Copy link
Author

@josevalim sounds good! if it is ok @dgud also for you I will modify it

@dgud
Copy link
Contributor

dgud commented Jul 4, 2017

Test that is doesn't crash if observing an old node, from which the new info is not supplied.

Also test the other way around, old observer receiving the new info,
if that works rebase to maint for inclusion in 20.1.

@Gsantomaggio
Copy link
Author

@dgud:

New to old:

➜  otp_2_ob git:(otp_observer_add_info) ✗ ./bin/erl -sname observer
Erlang/OTP 20 [RELEASE CANDIDATE 1] [erts-9.0] [source-0ab4a336d] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:10] [hipe] [kernel-poll:false]

Eshell V9.0  (abort with ^G)
(observer@mac)1> observer:start().
ok

The observer is:

new

Attach to an old version:

old

Old to new

screen shot 2017-07-04 at 17 02 59

there are not errors or crash

@Gsantomaggio
Copy link
Author

@dgud @josevalim WDYT ?

➜  otp_ob git:(otp_observer_add_info) ✗ bin/erl -sname observer +t 30000 +P 1024 +Q 2048
Erlang/OTP 20 [RELEASE CANDIDATE 1] [erts-9.0] [source-9b9ef7765] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:10] [hipe] [kernel-poll:false]

Eshell V9.0  (abort with ^G)
(observer@mac)1> observer:start().

screen shot 2017-07-04 at 22 19 31

@josevalim
Copy link
Contributor

@Gsantomaggio great! Shouldn't you flip the values though? 11903 / 30000 instead of 30000 / 11903? Especially as the area title says "statistics / limit"?

@Gsantomaggio
Copy link
Author

Ooops yes:) @josevalim
screen shot 2017-07-04 at 22 34 53

@Gsantomaggio
Copy link
Author

bin/erl -sname observer +t 30000 +P 1024 +Q 2048
Erlang/OTP 20 [erts-9.0.1] [source-3de78d02d] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:10] [hipe] [kernel-poll:false]

Eshell V9.0.1  (abort with ^G)
(observer@mac)1> observer:start().

Global view:

screen shot 2017-07-05 at 09 38 12

New observer connected to old node version

new_to_old

Old node connected to new observer node

old_to_new

@dgud
Copy link
Contributor

dgud commented Jul 6, 2017

Could you rebase this maint branch.

@Gsantomaggio Gsantomaggio changed the base branch from master to maint July 6, 2017 07:25
@Gsantomaggio
Copy link
Author

@dgud already did it. Just changed the PR to maint

@dgud
Copy link
Contributor

dgud commented Jul 6, 2017

Oh ok, thanks.

@dgud
Copy link
Contributor

dgud commented Jul 6, 2017

I don't like that you do the string construction in the backend, everywhere else the rawdata is sent,
and the string created in the gui, so I want that code in observer and not in observer_backend.

I think it's better to send the usage and limit as separate key-value pairs from backend and process the info in observer see example get_gc_info/1 in observer_procinfo of using a fun to fill in the info fields.

@Gsantomaggio
Copy link
Author

@dgud yes you are right. Let me change it

@Gsantomaggio
Copy link
Author

Gsantomaggio commented Jul 6, 2017

ok @dgud that's much better and it is fully compatible with the old versions:
screen shot 2017-07-06 at 18 35 55

just one question:
when you use the new to the old is:
screen shot 2017-07-06 at 18 36 11

Should I replace undefined with something else?

@dgud
Copy link
Contributor

dgud commented Jul 6, 2017

Maybe "Not available" is better?

@Gsantomaggio
Copy link
Author

@dgud done:

screen shot 2017-07-06 at 23 33 31

Copy link
Contributor

@dgud dgud left a comment

Choose a reason for hiding this comment

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

Otherwise looks good, I will take vacation it in August will merge when I get back.

wxSizer:layout(Sizer).


maybe_convert(V) when (is_atom(V) andalso V =:= undefined) -> "Not available";
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe_convert(undefined) -> "Not available";

io_lib:format("~s / ~s ~s",
[maybe_convert(C), maybe_convert(L),
if
{C, L} =:= {undefined, undefined} -> "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace with (otherwise C/L will crash if one of them is undefined)
C =:= undefined -> "";
L =:= undefined -> "";

@Gsantomaggio
Copy link
Author

@dgud done.
Enjoy your holidays! :)

@dgud dgud added the testing currently being tested, tag is used by OTP internal CI label Aug 10, 2017
wxSizer:add(Sizer, HSizer2, [{flag, ?wxEXPAND bor BorderFlags bor ?wxBOTTOM},
{proportion, 0}, {border, 5}]),

wxPanel:setSizer(Panel, Sizer),
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation should be done with space and not tabs in new or changed code

Copy link
Author

Choose a reason for hiding this comment

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

Sorry @dgud,
It should be ok now!

Show the statistics, limits and percentage for atoms, processes,ports and ETS.
Backward compatibility with old versions, In case the valuse is missing it shows
the string Not available.
@dgud dgud merged commit 2383da3 into erlang:maint Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants