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

Set process label #7720

Merged
merged 1 commit into from Nov 17, 2023
Merged

Conversation

dgud
Copy link
Contributor

@dgud dgud commented Oct 6, 2023

Add (and use) proc_lib:set(and get)_label/1

Make it possible for the user to set a process_description which
can be used in tools and crash reports to identity processes
but it doesn't have to be unique, as an registered name needs to be.

The process description can any term, for example {worker, 1..N},
{pool_process, 1..N} or something entirely different to identify
process that use general code.

While at it optimize fetching process info, so we don't have to
(rpc:) call the process_info(..) several times.

@dgud dgud self-assigned this Oct 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2023

CT Test Results

       5 files     155 suites   1h 49m 45s ⏱️
3 400 tests 3 098 ✔️ 302 💤 0
3 925 runs  3 571 ✔️ 354 💤 0

Results for commit 39962c7.

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

@dgud dgud force-pushed the dgud/stdlib/process-description/OTP-18789 branch from 1dbd45d to 03ad472 Compare October 9, 2023 08:01
@rickard-green rickard-green added the team:PS Assigned to OTP team PS label Oct 9, 2023
@dgud dgud force-pushed the dgud/stdlib/process-description/OTP-18789 branch from 03ad472 to 6719c90 Compare October 9, 2023 09:49
@dgud dgud requested a review from u3s October 9, 2023 12:32
@dgud dgud added the testing currently being tested, tag is used by OTP internal CI label Oct 9, 2023
Copy link
Contributor

@u3s u3s left a comment

Choose a reason for hiding this comment

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

Some comments dropped inline.

I didn't comment on commented out code. I assumed you are still developing and it is needed for reference. If not really needed, should be removed before merging I guess.

lib/stdlib/src/c.erl Show resolved Hide resolved
lib/stdlib/src/proc_lib.erl Outdated Show resolved Hide resolved
lib/stdlib/src/proc_lib.erl Outdated Show resolved Hide resolved
lib/stdlib/src/proc_lib.erl Show resolved Hide resolved
lib/stdlib/src/proc_lib.erl Outdated Show resolved Hide resolved
lib/stdlib/src/proc_lib.erl Outdated Show resolved Hide resolved
lib/stdlib/src/proc_lib.erl Outdated Show resolved Hide resolved
lib/stdlib/src/proc_lib.erl Outdated Show resolved Hide resolved
@dgud dgud force-pushed the dgud/stdlib/process-description/OTP-18789 branch from a10a48e to a0e532f Compare October 11, 2023 09:36
Copy link
Contributor

@garazdawi garazdawi left a comment

Choose a reason for hiding this comment

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

LGTM. I've added some minor comments.

As with all great features this leaves me wanting more :D

Some of the things I would like are:

  • Ancestors shown with their process description (both for self() and neighbors) in crash reports
  • Links shown with their process description (both for self() and neighbors) in crash reports
  • A convenience SpawnOpt to all gen behaviours to set the process description. This both makes it easier to set and exposes the functionality to users so that they know it exists without digging in the proc_lib docs.

PS. Are there really no tests for appmon_info, observer_backend and c:i? :( DS.

lib/observer/doc/src/observer_ug.xml Outdated Show resolved Hide resolved
lib/stdlib/src/proc_lib.erl Outdated Show resolved Hide resolved
lib/stdlib/src/proc_lib.erl Outdated Show resolved Hide resolved
lib/stdlib/test/proc_lib_SUITE.erl Outdated Show resolved Hide resolved
lib/stdlib/doc/src/proc_lib.xml Outdated Show resolved Hide resolved
lib/stdlib/doc/src/proc_lib.xml Outdated Show resolved Hide resolved
@dgud dgud force-pushed the dgud/stdlib/process-description/OTP-18789 branch 3 times, most recently from eecd9aa to 6317d72 Compare October 18, 2023 08:08
Copy link
Contributor

@u3s u3s left a comment

Choose a reason for hiding this comment

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

Great PR! Thanks.

Comment on lines 715 to 731
fetch_name([], Info) ->
case fetch({dictionary, '$process_label'}, Info) of
undefined -> "";
Id -> format_name(Id)
end;
fetch_name(Reg, _) ->
Reg.

format_name(Id) when is_list(Id); is_binary(Id) ->
case unicode:characters_to_binary(Id) of
{error, _, _} ->
io_lib:format("~0.tp", [Id]);
BinString ->
BinString
end;
format_name(TermId) ->
io_lib:format("~0.tp", [TermId]).
Copy link
Contributor

@u3s u3s Oct 18, 2023

Choose a reason for hiding this comment

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

this part is about fetching and formatting label. at a glance, name might suggest registered name.

@dgud dgud force-pushed the dgud/stdlib/process-description/OTP-18789 branch from 6317d72 to 39962c7 Compare October 19, 2023 10:59
sort_name(#etop_proc_info{name={_,_,_}}, _) ->
false.

%% sort_name(#etop_proc_info{name=Reg}, #etop_proc_info{name={M,_F,_A}})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we keeping this code as a comment?

@dgud dgud changed the title Set process description Set process label Nov 6, 2023
@rlipscombe
Copy link
Contributor

If I use proc_lib:set_label(?MODULE), then -- in observer -- it's hard to tell the difference from register(?MODULE, self()).

Do we need a way to denote that these are labels, rather than registered names?

@dgud
Copy link
Contributor Author

dgud commented Nov 7, 2023

Does it matter? It identifies the process(s).
If it is needed we can change that later.

@rlipscombe
Copy link
Contributor

Possibly not.

@rlipscombe
Copy link
Contributor

A bug, though -- probably in observer, rather than this: If two children have the same label, observer merges the nodes together.

@dgud
Copy link
Contributor Author

dgud commented Nov 7, 2023

A bug, though -- probably in observer, rather than this: If two children have the same label, observer merges the nodes together.

Where? In which tab?

@rlipscombe
Copy link
Contributor

Where? In which tab?

In the "Applications" tab. When I click on my application, all of my poolboy workers are represented by the same object (I assume because they've all got the same label). If I remove the label, they go back to separate objects.

@rlipscombe
Copy link
Contributor

@rlipscombe
Copy link
Contributor

Oh, and on the earlier observation, note that I was also talking about the applications tab (it's my favourite!), in case that makes a difference about whether it matters.

@dgud
Copy link
Contributor Author

dgud commented Nov 10, 2023

@rlipscombe I have pushed a version it kind of solves both your issues, the test app was useful thanks.

@dgud dgud changed the base branch from maint to master November 17, 2023 08:33
Make it possible for the user to set a process label which
can be used in tools and crash reports to identity processes
but it doesn't have to be unique, as an registered name needs to be.

The process label can any term, for example {worker, 1..N},
{pool_process, 1..N} or something entirely different to identify
process that use general code.

While at it optimize fetching process info, so we don't have to
(rpc:) call the process_info(..) several times.
@dgud dgud force-pushed the dgud/stdlib/process-description/OTP-18789 branch from eba3d1b to c4ae057 Compare November 17, 2023 10:02
@dgud dgud merged commit 106765e into erlang:master Nov 17, 2023
1 check passed
@@ -159,6 +159,17 @@
</desc>
</func>

<func>
<name name="get_label" arity="1" since="OTP 26.2"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm interested to learn if this will get included in the upcoming 26.2? It seems this branch was merged to master instead of maint, but the docs say since 26.2 here and for set_label/1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No master only, we changed our mind and forgot to update the docs, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:PS Assigned to OTP team PS 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

9 participants