Display value of '$process_label' in observer

[rlipscombe]

In http://erlang.org/pipermail/erlang-questions/2021-May/100972.html, it was suggested (after some discussion) that non-unique process labels would make using observer nicer. I followed this up here: http://erlang.org/pipermail/erlang-questions/2021-June/101152.html

This is a first stab at implementing that, for feedback.

It defines a convention where the process dictionary can contain '$process_label', which is assumed to be an Erlang string (list of chars). If there's interest in the concept, I'll look at extending gen_server et. al. to support this through options.

I'm currently using it to make it easier to observe a particularly nested supervision tree.

[CLAassistant]

All committers have signed the CLA.

[michalmuskala]

Would it make sense to support any term in there and use io_lib:format("~p") on it? I could imagine something like {service_x, 1} for some sort of sharded service. A tuple like that would still be very readable in observer output

[galdor]

I would expect this kind of property to at least support strings, binaries and tuples: as Michał said, there are multiple reasons to use a multi-valued label, e.g. {acceptor, <name>}, {db_client, <id>}…).

Note that for tuples, ~tp should always be used: Erlang does not support non-latin1 characters by default, even if your system uses a UTF-8 locale (and I have no hope for this to ever change).

[rlipscombe]

I've updated the commit to allow process labels to be any term.

[galdor]

You need to separate formatting of strings (lists or binaries) and other terms; using ~p for strings will return quoted values (e.g. "foo" and <<"foo">>).

Something such as:

case proplists:get_value('$process_label', D) of
    undefined ->
        pid_to_list(P);
    Label when is_list(Label); is_binary(Label) ->
        io_lib:format("~ts ~tp", [Label, P]);
    Label ->
        io_lib:format("~tp ~tp", [Label, P])
end;

[dgud]

So we agreed that this is useful information but it was decided that we should have
an api for setting and getting this information, so that where (and how) the data is
stored can be changed.

'proc_lib' or 'sys' might be a good place for this functionality with set/get_process_description/1.

[rlipscombe]

'proc_lib' or 'sys' might be a good place for this functionality with set/get_process_description/1.

Is there a compelling reason to choose one over the other? Based on a quick skim through the docs, it seems to me that 'sys' would be the better place, since it's already got "inspection"-type stuff in it.

[dgud]

We have not discussed the name or where to put them it was just a suggestion, we leave that to you
for now and will take that discussion when a new (or updated) PR arrives :-)

[rlipscombe]

Just to be clear: I've not forgotten about this; been busy with the day job.

[rlipscombe]

I've updated the PR. See the new commit.

[nathanl]

Still wishing and hoping for this to land. ✨ ✨ ✨

[rlipscombe]

I've got time to work on this now, but I'm waiting for some feedback on the implementation.

[dgud]

Yes, I have been thinking on this on and off, and don't know what to do about it.

What I want to avoid is a performance load on the target node, and as it is today getting the
process_dictionary on another process can be expansive (if large).

Now as it is written in the current PR, it is used in observer in the application tab only,
which is almost never updated and only fetched for the displayed application processes
and that should be ok.

But if we add it there people will want it in the 'Processes' tab, at least I want it there, (which works as 'top') and then it will be fetched on every update (1-10s) and for all processes.
We already fetch the '$initial_call' there which will do a second copy of the dictionary.

[rlipscombe]

If we discard the idea of sys:get_label, etc., and lean into using the process dictionary, can we make only one copy of the dictionary?

I prefer using a well-known value in the process dictionary because it removes short-term compatibility concerns: I can decorate my code with labels without worrying about which version of OTP I'm targetting. It adds coupling in the long term, though.

[dgud]

We decided to use a functional interface so we can change it later to something with more performance.

But maybe move the functions to proc_lib and add a new function there,
that lookup the label and if it does not exist lookup the initial call.

[github-actions]

CT Test Results

       3 files       96 suites   38m 40s ⏱️
1 894 tests 1 832 ✔️ 62 💤 0 ❌
2 190 runs  2 126 ✔️ 64 💤 0 ❌

Results for commit f876043.

♻️ 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

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• Windows Installer

// Erlang/OTP Github Action Bot

[rlipscombe]

But maybe move the functions to proc_lib.

Can do, but...

add a new function there, that lookup the label and if it does not exist lookup the initial call

I don't like this. The fallback from the label to the initial call feels like an observer concern, and it feels like we'd be polluting proc_lib with it.

as it is today getting the process_dictionary on another process can be expansive (if large).

This feels like it needs further discussion. If we provided a way to get only some of the process dictionary, and if we continued to keep the process label in the dictionary, something like erlang:get_process_dictionary(Pid, Keys) would address that concern. I know you've talked about a functional API, but proc_lib keeps a bunch of other things in the dictionary, so keeping the label in there doesn't seem like much of a departure.

observer could use it like this:

Term = case erlang:get_process_dictionary(Pid, ['$process_label', '$initial_call']) of
    [L, _] when L =/= undefined -> L;
    [_, IC] when IC =/= undefined -> IC;
    _ -> undefined
end

However: is there any evidence that large process dictionaries are common, and how much impact does it really have...?

[paulo-ferraz-oliveira]

Today I ran into an issue that would benefit from this (process label on Observer). Is this pull request still being considered? I see it as marked Stalled, but don't know why.

[dgud]

Not in the current form, but stalled means we need to take a look at this but it is down on the priority list.

[rlipscombe]

Stalled, but don't know why.

That's on me. I stepped away from working with Erlang for a few months. Nudging discussion slightly:

I don't like this. The fallback from the label to the initial call feels like an observer concern, and it feels like we'd be polluting proc_lib with it.

I've thought about this some more, and I'm thinking that proc_lib could use the initial call as a default for the label, if it's not specified. That's neater.

So it'd look something like this:

1. The various spawn functions in proc_lib could take an optional label parameter. If not specified, it would use the initial call as the label.
2. For now (implementation detail) the label goes in the process dictionary.
3. proc_lib provides functions to get/set the label.
4. observer uses the function to get the label.

It's relatively low-impact, hides the implementation details, and is easy to change later.

[rlipscombe]

I've looked into a couple of options:

1. Putting this in proc_lib:spawn_opt -- that's brittle, 'cos init_p is exported as-is, relied on by a few other things, and I have no way of knowing if other code relies on it.
2. Addressing the dictionary performance thing, I think erlang:process_dictionary(Pid) and erlang:process_dictionary(Pid, KeyOrKeys) would be really useful. But I looked at the BIF code for process_info/2 and I do not want to go anywhere near that stuff without a run-up.

So: I think the least-bad option at this point is:

1. to go with sys:put_label/1 and sys:get_label/0,1. For now, those can use the process dictionary (but as an implementation detail, not to be relied on). If someone is brave enough to add erlang:process_dictionary/1,2 all well and good.
2. Observer to not fall back to $initial_call because of the performance concern (but I could be talked round).
3. Follow-up PRs to get gen_foo.erl to take a label option and call sys:put_label.

This is basically the state of the PR right now. If that's not good enough, then I'm out of ideas.

[nathanl]

Although "observer" is in the issue title, I like that this code is not Observer-specific. These labels could be useful in Observer itself and also other tools like it, such as Phoenix.LiveDashboard's process view

[dgud]

Replaced by #7720