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 system monitor support #32

Merged
merged 8 commits into from
Feb 11, 2025
Merged

Add system monitor support #32

merged 8 commits into from
Feb 11, 2025

Conversation

mrkirby153
Copy link
Member

Adds support for receiving system monitor events from :erlang.system_monitor.

There are two emitters by default:

  • An emitter that emits metrics for events
  • An emitter that logs events to the standard logger.

ref = Map.filter(state.subscribers, fn {_, p} -> p == pid end) |> Map.keys()

state =
case ref do
Copy link

Choose a reason for hiding this comment

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

If one pid calls subscribe more than once, ref will end up being a list with more than one element, and no unsubscription will be performed here. I would rewrite this function to something like

  state = 
    state.subscribers
    |> Enum.filter(state.subscribers, fn {_, p} -> p == pid end)
    |> Enum.reduce(state, fn {ref, _pid}, state ->
        Process.demonitor(ref)
        %__MODULE__{state | subscribers: Map.delete(state.subscribers, ref)
    end)

{:reply, :ok, state}

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not possible for pid to be in the map more than once, subscribe/1 checks if the pid is already subscribed, if it is, it no-ops.

Copy link

Choose a reason for hiding this comment

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

Ah you're right. I still think it's a bit cleaner to handle it as a list still but that's just my 2c. Won't block on it

@mrkirby153 mrkirby153 merged commit 8e4e93b into master Feb 11, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants