-
Notifications
You must be signed in to change notification settings - Fork 1
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
using system diagnostics for logging #59
base: main
Are you sure you want to change the base?
Conversation
RelationalAI.Test/UnitTest.cs
Outdated
@@ -52,6 +53,11 @@ public Client CreateClient() | |||
{ | |||
httpClient.DefaultRequestHeaders.Add(header.Key, header.Value); | |||
} | |||
|
|||
// system diagnostics logging configuration | |||
testClient.TraceSource.Listeners.Add(new ConsoleTraceListener()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this in principle, if we want to conform the logs with XUnit's logger (might be good for timing data and conforming format) we can create a new trace listener here, take the ITestOutputHelper in the ctor and use that to log out trace events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
RelationalAI.Test/UnitTest.cs
Outdated
|
||
// system diagnostics logging configuration | ||
testClient.TraceSource.Listeners.Add(new ConsoleTraceListener()); | ||
testClient.TraceSource.Switch.ShouldTrace(TraceEventType.Verbose); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to think about: How should we parameterize the verbosity.. here we hardcode it to verbose which is fine to start, but is that always useful
RelationalAI/Client.cs
Outdated
@@ -50,13 +53,19 @@ public HttpClient HttpClient | |||
set { _rest.HttpClient = value; } | |||
} | |||
|
|||
public TraceSource TraceSource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to research - right now we think we can only do file-based configuration in .NET framework, but we need it in .NET core, how should we resolve this so programmatic configuration is not required?
Possibilities:
Our own config file interpretation
XUnit's config file
(understanding how this interplays with other environments the SDK is being run in such as ASP.NET core server)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People complains about the missing ability to configure diagnostics traces using app.config or a similar configuration file, dotnet/runtime#23937
keep digging around, hopefully we won't need a custom config file interpretation 🙏
RelationalAI/Client.cs
Outdated
public Task<Database> CreateDatabaseAsync(string database, string engine) | ||
{ | ||
return CreateDatabaseAsync(database, engine, false); | ||
} | ||
|
||
public async Task<Database> CreateDatabaseAsync(string database, string engine, bool overwrite) | ||
{ | ||
_traceSource.TraceEvent(TraceEventType.Verbose, 0, $"creating database {database}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can just use some 100-offset number for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
No description provided.