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

Prefix the library logs #74

Closed
wants to merge 3 commits into from
Closed

Conversation

bokner
Copy link
Contributor

@bokner bokner commented Aug 18, 2023

This change will prefix the logs emitted by this library. The intent is to make it easier to identify them in the log output. Currently, the prefix is hardcoded in utils.ex.

Also, there may be a better way of doing things. I had to use two clauses for MLLP.Utils.format_message/1, as ExUnit.CaptureLog.capture_log/2 sometimes truncates the output, which could result in failing tests. More specifically, if you comment out the first clause of MLLP.Utils.format_message/1, one of the tests in 'client_test.exs` will fail.

Edit: @starbelly found the issue, and that was a missing 'inspect' in the client's logs.

@@ -0,0 +1,15 @@
defmodule MLLP.Utils do
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might want to get rid of this wrapper and keep the Logger calls as close to the call site as possible so that that information captured by Logger doesn't start here, maybe not a huge deal though either.

Other part, we want the prefix to be contextual. Users can already configure the logger to emit log lines with the application name (i.e., mllp). However, even if they do, there some context lost (i.e., did a error come from the client or receiver?).

So, perhaps we prefix with MLLP.Client or MLLP.Receiver, thoughts?

Copy link
Contributor Author

@bokner bokner Aug 19, 2023

Choose a reason for hiding this comment

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

Agreed, having a hardcoded prefix for everything is not good.
019cba9 tries to address it by passing an optional prefix to MLLP.Utils.log/3. I made changes to MLLP.Client and MLLP.Receiver, so all their logs use module names. Logs for other modules will stay as they were unless we want to change them.

Choose a reason for hiding this comment

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

This looks good to me, if you decide to go down this route, but also note that Logger includes both module, MFA, and application in its metadata. So if you include the application in the metadata, you would see application=mllp in all relevant log entries.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but we'll need to ditch the wrapper I do believe, as using it will negate that option. But I hear you, maybe we just don't prefix anything and leave it to the user to configure this based on your suggestion. Still we can use this PR to improve upon the messages themselves (also need to refresh myself on the state of structured logging in elixir now).

@starbelly starbelly closed this Oct 14, 2023
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.

3 participants