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

Use Monotonic time for calculating request latency in Plug.Logger #373

Merged
merged 2 commits into from Mar 30, 2016

Conversation

mje113
Copy link

@mje113 mje113 commented Mar 30, 2016

According to http://erlang.org/doc/apps/erts/time_correction.html#Erlang_Monotonic_Time to Measure Elapsed Time:

Take timestamps with erlang:monotonic_time/0 and calculate the time difference using ordinary subtraction. The result will be in native time unit. If you want to convert the result to another time unit, you can use erlang:convert_time_unit/3.

An easier way to do this is to use erlang:monotonic_time/1 with the desired time unit. However, you can then lose accuracy and precision.

@mje113
Copy link
Author

mje113 commented Mar 30, 2016

I just realized that Plug is supposed to be backward compatible to Elixir 1.0, so... :erlang.monotonic_time/0 isn't available.

@whatyouhide
Copy link
Member

@mje113 you could do something like we did in Phoenix in this PR (the current Phoenix master code doesn't do this anymore).

@mje113
Copy link
Author

mje113 commented Mar 30, 2016

@whatyouhide Ah yes, good point. I also came across elixir-lang/elixir#4024 (by you), so it looks like System.monotonic_time/0 is also supported in later releases. Stylistically what's favored in Elixir?

@whatyouhide
Copy link
Member

As you said, we can't straight use :erlang.monotonic_time/0 because it's only in newer versions of Erlang; for the same reason, we can't use System.monotonic_time/0 as it's only in newer versions of Elixir 😺 You could check for System.monotonic_time/0 if you follow the pattern in the PR I linked, but I'd say :erlang.monotonic_time/0 is better as the Elixir version requires new versions of both Elixir/Erlang.

@mje113
Copy link
Author

mje113 commented Mar 30, 2016

Got it, that makes sense. Commit incoming.

@lexmag
Copy link
Member

lexmag commented Mar 30, 2016

@mje113 please squash everything into single commit.

@@ -41,6 +42,15 @@ defmodule Plug.Logger do
end)
end

# TODO: remove this once Plug supports only Elixir 1.2.
if function_exported?(:erlang, :monotonic_time, 0) do
defp current_time, do: :erlang.monotonic_time
Copy link
Member

Choose a reason for hiding this comment

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

We should use :erlang.monotonic_time(:micro_seconds) here and remove convert_time_unit call.

Copy link
Member

Choose a reason for hiding this comment

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

Let's not do that because we lose precision. The ideal is to always convert later.

@josevalim josevalim merged commit 4f486ae into elixir-plug:master Mar 30, 2016
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

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.

None yet

4 participants