Skip to content

Conversation

@roji
Copy link
Member

@roji roji commented Jun 8, 2022

This is based off of #4492, so only review the latest commit.

Closes #4493

@roji roji marked this pull request as ready for review June 8, 2022 14:10
@roji roji requested review from Brar and vonzshik as code owners June 8, 2022 14:10
@roji roji force-pushed the DataSource/Logging branch from bcf6ef0 to b9a7397 Compare June 8, 2022 14:13
catch (Exception e)
{
Logger.LogError(e, $"Exception caught in {nameof(SingleThreadSynchronizationContext)}");
Trace.Write($"Exception caught in {nameof(SingleThreadSynchronizationContext)}:" + Environment.NewLine + e);
Copy link
Member Author

Choose a reason for hiding this comment

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

Note this problematic thing - SingleThreadSynchronizationContext is the only place in the codebase where we log from a truly static context, so we can no longer use regular logging there.

We could in theory have a STSC-per-datasource, but this really is a very rare case which has never yet been reported (exception bubbling up) - so it doesn't seem important enough (but let me know if you think otherwise). If we suspect a user is running into it, we can instruct them how to get the message traced here.

Copy link
Member

Choose a reason for hiding this comment

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

Given that this is really an exceptional case (not part of a normal flow of operations) I don't think it's strange this goes into trace or some named diag source

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, agree.

@roji roji force-pushed the DataSource/Logging branch from b9a7397 to 8e42612 Compare June 19, 2022 20:59
Copy link
Member

@NinoFloris NinoFloris left a comment

Choose a reason for hiding this comment

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

LGTM, nice to see it being used in all the tests too!

@roji roji merged commit 00ee7a8 into npgsql:main Jun 21, 2022
@roji roji deleted the DataSource/Logging branch June 21, 2022 19:30
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.

Integrate logging into NpgsqlDataSource

2 participants