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

Controlling process #41

Merged
merged 2 commits into from
May 18, 2016
Merged

Controlling process #41

merged 2 commits into from
May 18, 2016

Conversation

scohen
Copy link
Collaborator

@scohen scohen commented May 18, 2016

When used in code that's called a lot, Elixometer's main process can become a bottleneck. I witnessed this in our rate limiter, where we would send quite a few performance metrics through Elixometer, which would negatively affect the 99% response time of the app.

The cause of this is Elixometer's synchronous gen_server, which waits until exometer responds before returning. This PR creates another gen_server, the Updater that does the work of notifying exometer. The updater also uses the po_box library to ensure its mailbox doesn't overflow. This means that Elixometer can drop messages, but I think that's a better compromise than having it affect your system's performance.

@jparise

|> Enum.map(&(:exometer_report.subscribe(reporter, name, &1, interval)))
end
:ets.insert(@elixometer_table, {{:subscriptions, name}, true})
def ensure_registered(name, register_fn) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could use some documentation. It took me a minute to realize what "name" is here and how subscribe(name) (called within) differs from subscribe_to(name).

@jparise
Copy link
Collaborator

jparise commented May 18, 2016

Just some minor comments. Looks good overall!

In one of our high-throughput systems, timing messages were a bottleneck
because the update calls were synchronous. This PR moves the calls to an
updater process that is capped at 1000 messages in its queue using the
pobox framework. This should alleviate the bottleneck and prevent the
process mailboxes from getting too big.
@scohen scohen force-pushed the controlling_process branch from c18e1e9 to 5a06b48 Compare May 18, 2016 17:50
@scohen scohen force-pushed the controlling_process branch from 5a06b48 to 4fe9234 Compare May 18, 2016 17:57
@scohen scohen merged commit 9dae8d8 into pinterest:master May 18, 2016
@scohen scohen deleted the controlling_process branch May 18, 2016 18:29
@scohen scohen restored the controlling_process branch May 26, 2016 02:43
annard pushed a commit to Intellicore/elixometer that referenced this pull request Oct 11, 2016
* Addressing bottleneck

In one of our high-throughput systems, timing messages were a bottleneck
because the update calls were synchronous. This PR moves the calls to an
updater process that is capped at 1000 messages in its queue using the
pobox framework. This should alleviate the bottleneck and prevent the
process mailboxes from getting too big.

* Addressed PR comments
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