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

Logger handles :process_label from OTP27 #13392

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

sabiwara
Copy link
Contributor

@sabiwara sabiwara commented Mar 5, 2024

OTP27 introduces a new :process_label key between :registered_name and :error_info (erlang/otp#7720), breaking the pattern-matching on the keyword-list:
Screenshot 2024-03-05 at 20 34 29

This PR just fixes the CI, but doesn't do anything with the label. We could maybe display it if != :undefined?

Comment on lines +409 to +411
{pid, crashed} = Keyword.pop_first(crashed, :pid)
{name, crashed} = Keyword.pop_first(crashed, :registered_name)
{{kind, reason, stack}, crashed} = Keyword.pop_first(crashed, :error_info)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this should be semantically more robust to future changes, while still efficient enough popping from the head?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is totally fine, it's a really tiny list.

Copy link
Member

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

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

Looks great. Do we have a test for this that we can run only on OTP27?

Comment on lines +409 to +411
{pid, crashed} = Keyword.pop_first(crashed, :pid)
{name, crashed} = Keyword.pop_first(crashed, :registered_name)
{{kind, reason, stack}, crashed} = Keyword.pop_first(crashed, :error_info)
Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is totally fine, it's a really tiny list.

@sabiwara
Copy link
Contributor Author

sabiwara commented Mar 5, 2024

Do we have a test for this that we can run only on OTP27?

Sorry I am not sure what you mean.
The current change is the strict minimum for the existing code (and tests) to work both on OTP26- and OTP27+, but I'm not sure how we could add an extra test for OTP27.

What I can think of is:

  • add ProcessLabel: #{inspect(label)} if label != :undefined
  • add a test setting the label and run this test only on OTP27

It would be a change of behavior but might make sense to include it, based on the doc from erlang/otp#7720:

The primary purpose is to aid in debugging unregistered processes.
The process label can be used in tools and crash reports to identify processes
but it doesn't have to be unique or an atom, as a registered name needs to be.
The process label can be any term, for example {worker_process, 1..N}.

@whatyouhide
Copy link
Member

Perfect, that's what needed to know then 👍 No need for extra tests 🙃

@sabiwara
Copy link
Contributor Author

sabiwara commented Mar 5, 2024

Perfect, that's what needed to know then 👍 No need for extra tests 🙃

Awesome, thank you for confirming!

@sabiwara sabiwara merged commit 4f66663 into elixir-lang:main Mar 5, 2024
8 checks passed
@sabiwara sabiwara deleted the logger-otp27 branch March 5, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants