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

QoL fixes #36

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

QoL fixes #36

wants to merge 1 commit into from

Conversation

vans163
Copy link

@vans163 vans163 commented Nov 28, 2022

Remove Logger side effects
Fix empty lines crashing
Fix >1 "=" on the line
Fix path vs filename confusion

Remove Logger side effects
Fix empty lines crashing
Fix >1 "=" on the line
Fix path vs filename confusion
Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

Hi @vans163, 👋
Thank you for opening this PR with your feedback. 👌
A few of these code enhancements look like they make sense.
But we tend to prefer the creation of an Issue to discuss before creating a PR. 💭

When we originally investigated the options for importing environment variables in Elixir: dwyl/learn-environment-variables#18 we concluded that they weren't very beginner friendly. 😞
One of the reasons we decided to create our own env importing package
was precisely to have Logger output so that when a variable is not present,
we get a friendly debug/warning/error message. 👌

We very much want to keep these messages.

@vans163
Copy link
Author

vans163 commented Nov 30, 2022

Feel free to pick some stuff out if anything is useful. Elixir Logger is a problem for us in prod (it doesn't scale as all msgs to go 1 process) and we need precise control over all IO; actually we have our own logger. This was a bit rushed with the Logger but your repo is GNU license :) (even if it wasn't I would still contribute back a few internal fixes we made, its the right thing to do)

@nelsonic
Copy link
Member

Very curious about what you're saying regarding the Logger not scaling in Prod. 💭
Is this documented anywhere public e.g. a blog post, forum topic or StackOverflow Q/A? (really want to read more!) 👀
I think a good thing to do would be make the logs configurable in config/{env}.exs file ... 💡

@vans163
Copy link
Author

vans163 commented Nov 30, 2022

Very curious about what you're saying regarding the Logger not scaling in Prod. thought_balloon Is this documented anywhere public e.g. a blog post, forum topic or StackOverflow Q/A? (really want to read more!) eyes I think a good thing to do would be make the logs configurable in config/{env}.exs file ... bulb

Freds work is/(was?) logging at Heroku he often has good opinions like https://ferd.ca/erlang-otp-21-s-new-logger.html.

I dont have anything solid on hand but one issue off the top of my head was general spam of crash reports so we had to rely on :proc_lib.spawn instead of :erlang.spawn and handle exit errors+crashes properly (or they would get lost).

Another was IO of a single gen_server doing logging cant keep up, we had to split process_groups/modules into individual loggers + files, imagine a game with 30k players connected concurrently, each logging every action that is taken like "walk to X" "attack with sword". Theres a module responsible for a marketplace in the game, guild chats, global chats, etc.

Third is the Elixir logger has its own formatting, sometimes you want to intercept that to apply custom formatting but the exposed interface for that is very narrow (maybe its gotten better in recent Elixlir).

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