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.

Sign up for GitHub

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

using system diagnostics for logging #59

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0219976
using system diagnostics for logging
NRHelmi Jan 22, 2023
0f4845e
custom test trace listener based on xunit test output helper
NRHelmi Jan 23, 2023
247ec3d
test output helper doesn't support write only
NRHelmi Jan 24, 2023
79bcbcb
add a logger abstraction
NRHelmi Jan 26, 2023
afb7ea1
fix
NRHelmi Jan 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions RelationalAI.Test/UnitTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Text;
Expand Down Expand Up @@ -52,6 +53,11 @@ public Client CreateClient()
{
httpClient.DefaultRequestHeaders.Add(header.Key, header.Value);
}

// system diagnostics logging configuration
testClient.TraceSource.Listeners.Add(new ConsoleTraceListener());
Copy link
Collaborator

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

testClient.TraceSource.Switch.ShouldTrace(TraceEventType.Verbose);
Copy link
Collaborator

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


return testClient;
}

Expand Down
14 changes: 13 additions & 1 deletion RelationalAI/Client.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Net.Http;
using System.Text;
Expand All @@ -37,11 +38,13 @@ public class Client
private const string PathOAuthClients = "/oauth-clients";
private readonly Rest _rest;
private readonly Context _context;
private readonly TraceSource _traceSource;

public Client(Context context)
{
_context = context;
_rest = new Rest(context);
_traceSource = new TraceSource("RAI", SourceLevels.All);
_rest = new Rest(context, _traceSource);
}

public HttpClient HttpClient
Expand All @@ -50,13 +53,19 @@ public HttpClient HttpClient
set { _rest.HttpClient = value; }
}

public TraceSource TraceSource
Copy link
Collaborator

@torkins torkins Jan 23, 2023 •
edited
Loading

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)

Copy link
Contributor Author

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 🙏

{
get { return _traceSource; }
}

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}");
Copy link
Collaborator

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

var mode = CreateMode(null, overwrite);
var transaction = new Transaction(_context.Region, database, engine, mode);
await _rest.PostAsync(MakeUrl(PathTransaction), transaction.Payload(null), null, transaction.QueryParams());
Expand Down Expand Up @@ -111,6 +120,7 @@ public async Task<DeleteDatabaseResponse> DeleteDatabaseAsync(string database)

public async Task<Engine> CreateEngineAsync(string engine, string size = "XS")
{
_traceSource.TraceEvent(TraceEventType.Verbose, 0, $"creating engine {engine} with size {size}");
var data = new Dictionary<string, string>
{
{ "region", _context.Region },
Expand Down Expand Up @@ -532,6 +542,7 @@ public async Task<TransactionAsyncResult> ExecuteAsync(
if (rsp is string s)
{
var txn = Json<TransactionAsyncCompactResponse>.Deserialize(s);
_traceSource.TraceEvent(TraceEventType.Verbose, 0, $"transaction id {txn.Id}");
return new TransactionAsyncResult(txn, new List<ArrowRelation>(), null, new List<object>());
}

Expand Down Expand Up @@ -741,6 +752,7 @@ private TransactionAsyncResult ReadTransactionAsyncResults(List<TransactionAsync
}

var transactionResult = Json<TransactionAsyncCompactResponse>.Deserialize(_rest.ReadString(transaction.Data));
_traceSource.TraceEvent(TraceEventType.Verbose, 0, $"transaction id {transactionResult.Id}");
var metadataProto = _rest.ReadMetadataProtobuf(metadata.Data);

List<object> problemsResult = null;
Expand Down
5 changes: 4 additions & 1 deletion RelationalAI/Rest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Net.Http;
Expand All @@ -37,11 +38,13 @@ public class Rest
private const string RequestIdHeaderName = "X-Request-ID";

private readonly Context _context;
private TraceSource _traceSource;

public Rest(Context context)
public Rest(Context context, TraceSource traceSource = null)
{
_context = context;
HttpClient = new HttpClient();
_traceSource = traceSource ?? new TraceSource("RAI", SourceLevels.All);
}

public HttpClient HttpClient { get; set; }
Expand Down