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.

Sign up for GitHub

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

Jump to bottom

Erlang/OTP 21 logger integration #7649

Merged
merged 5 commits into from
May 7, 2018
Merged

Erlang/OTP 21 logger integration #7649

merged 5 commits into from
May 7, 2018

Conversation

josevalim
Copy link
Member

@josevalim josevalim commented May 6, 2018 •
edited

No description provided.

@michalmuskala
Copy link
Member

Could we get the optimisations to Enum and Keyword in separate commit/PR? I think this will help keep the discussion focused.

@josevalim josevalim changed the title Initial work on new logger integration Erlang/OTP 21 logger integration May 6, 2018
@josevalim
Copy link
Member Author

This is ready for review. I just need to write tests for the new handler and it is ready to go.

@josevalim josevalim mentioned this pull request May 6, 2018
Closed
3 tasks
sirihansen
sirihansen reviewed May 7, 2018
View reviewed changes
lib/ex_unit/lib/ex_unit/capture_log.ex
@@ -70,7 +70,7 @@ defmodule ExUnit.CaptureLog do
{:ok, string_io} = StringIO.open("")

try do
_ = :gen_event.which_handlers(:error_logger)
_ = Process.whereis(:error_logger) && :gen_event.which_handlers(:error_logger)
Copy link

Choose a reason for hiding this comment

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

Just for info: there's a new error_logger:which_report_handlers() - which returns the empty list if error_logger process is not running. But can of course only be used if you know you're running on 21.

sirihansen
sirihansen reviewed May 7, 2018
View reviewed changes
lib/logger/lib/logger/erlang_handler.ex Outdated
defp erlang_level_to_elixir_level(:critical), do: :error
defp erlang_level_to_elixir_level(:error), do: :error
defp erlang_level_to_elixir_level(:warning), do: :warn
defp erlang_level_to_elixir_level(:info), do: :info
Copy link

Choose a reason for hiding this comment

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

Missing notice.

Copy link
Member Author

@josevalim josevalim May 7, 2018 •
edited

Choose a reason for hiding this comment

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

@sirihansen Thanks! I am glad you noticed that.

@josevalim josevalim merged commit 2c770ba into master May 7, 2018
@josevalim
Copy link
Member Author

❤️ 💚 💙 💛 💜

@josevalim
Copy link
Member Author

I have merged this because it causes some intermittent failures in our suite, making it really hard to work on other Erlang/OTP 21 features. Please drop any other feedback in this thread and I will get to it. :)

@josevalim josevalim deleted the jv-new-otp-logger branch May 7, 2018 12:46
@sirihansen
Copy link

Just one minor doc-thing - logger.ex, line 574 - should probably remove "both":

* `:flush` - when `true`, guarantees all messages currently sent
+      to both `Logger` are processed before the backend is removed

@josevalim
Copy link
Member Author

@sirihansen thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sirihansen sirihansen sirihansen left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@josevalim @michalmuskala @sirihansen