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

Include source in StreamLogHandler output #189

Merged
merged 1 commit into from Apr 6, 2021
Merged

Include source in StreamLogHandler output #189

merged 1 commit into from Apr 6, 2021

Conversation

slashmo
Copy link
Contributor

@slashmo slashmo commented Apr 6, 2021

Include source in StreamLogHandler output.

Motivation:

When using the same Logger across an application in combination with the default StreamLogHandler, it may be difficult to differentiate between logs produced by different modules. ServiceLifecycle works around this by prepending the log message with [Lifecycle], which is what I'd like to do StreamLogHandler by default, leveraging the source log parameter.

Modifications:

StreamLogHandler will include the source in brackets right before the log message when writing a log.

Result:

Logs from different modules are easily differentiable when produced using the same Logger instance.

@swift-server-bot
Copy link

Can one of the admins verify this patch?

8 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@slashmo
Copy link
Contributor Author

slashmo commented Apr 6, 2021

I added this PR as a starting point for a discussion around whether we should be including the source parameter in log output produced by StreamLogHandler. CC @ktoso, @weissi

@weissi
Copy link
Member

weissi commented Apr 6, 2021

I am personally happy with this PR, so good to go from my side :).

Said that, we kinda intentionally made StreamLogHandler "not good" because we want people to use real logging backends instead of relying on this "toy implementation". But given that we probably shouldn't omit too much information, I'd say let's take adding "source".

@weissi
Copy link
Member

weissi commented Apr 6, 2021

@swift-server-bot test this please

Motivation:

When using the same Logger across an application in combination
with the default StreamLogHandler, it may be difficult to differentiate
between logs produced by different modules. ServiceLifecycle works around
this by prepending the log message with [Lifecycle], which is what I'd like
to do StreamLogHandler by default, leveraging the source log parameter.

Modifications:

StreamLogHandler will include the source in brackets right before
the log message when writing a log.

Result:

Logs from different modules are easily differentiable when produced
using the same Logger instance.
@weissi
Copy link
Member

weissi commented Apr 6, 2021

@swift-server-bot add to allowlist

@slashmo
Copy link
Contributor Author

slashmo commented Apr 6, 2021

Said that, we kinda intentionally made StreamLogHandler "not good" because we want people to use real logging backends instead of relying on this "toy implementation".
@weissi

Good point! Although one could also argument that having it as a reference point for "better" implementations would also be beneficial, so including it could motivate other impls to do the same.

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

I like this and to @slashmo's point: We should educate logging backend builders to include source in their logs. One of the most effective ways is likely to just include it in the StreamLogger. So deffo +1 here

Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Yeah, LGTM from my end as well.

Our backend is known to try to be intentionally not great, but it should not drop data is has already. I was surprised we didn't log the source, so definitely +1.

Thanks a lot for noticing @slashmo !

@ktoso ktoso added this to the 1.4.3 milestone Apr 6, 2021
@ktoso ktoso merged commit bc049d0 into apple:main Apr 6, 2021
@slashmo slashmo deleted the stream-log-source branch April 6, 2021 13:13
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