search

aaOpenSource / aaLog

A library and example programs for reading the standard SMC log file format
MIT License
13 stars 15 forks source link

Add ClientID to GetUnreadRecords function #32

Closed arobinsongit closed 8 years ago

arobinsongit commented 8 years ago

The current codebase has a logical flow that prevents it from being utilized properly with the Web API.

The GetUnreadRecords function depends on the writing of a cache file so that subsequent calls to the same function can be made without the client itself having to keep track of the last read record. This design was created to allow for a simplification on the client side where the client is only required to call GetUnreadRecords with no other arguments and the newest records will always be returned. This will typically not cause an issue when only a single client is on a machine or when different processes are calling different instances of the library.

The issue with the Web API is that a single process on a single machine with the remote clients being unique. A new parameter was added to the GetUnreadRecords logical call to allow the client to pass a unique client id. The client will pass this id via the request header attribute clientid when it makes the call to get the unread records. If no client id is passed then a GUID is generated using the NewGuid() function and that new clientid is used in the call to get the unread records. Key in this method is that the clientid used for the call is always returned in the response headers. This way the calling client can make the first call with no clientid but on subsequent calls the client should pass the returned clientid back via the request header.

I have added units tests to handle this new functionality as well as provide some coverage for base GetUnreadRecords functionality.

Inclusion of this PR in combination with PR #30 should resolve Issue #26

arobinsongit commented 8 years ago

@ryanvs and @logic-danderson take a look at this PR and let me know your thoughts. This was the best method I have come up with for dealing with the issue of multiple clients accessing a singleton class via the web API for doing a GetUnreadRecords call.

ryanvs commented 8 years ago

I have only looked at the code diff on github and haven't had a chance to load it into Visual Studio yet. I'm concerned about the Use Case for GetUnreadRecords though. When I envision the API, I think that the client needs to maintain the last state information and aaWebApi needs to pass enough return information in order for that to be possible. So the scenario would be something like:

  1. Client requests the latest log event (position) and log file state information
  2. Client can optionally request x historical log events
  3. Client enters a loop, asks for any changes since last state and updates the state information

Sorry to be vague, but I haven't had time to prototype anything. I'm also considering scenarios where the client would be aggregating logs from multiple sources - most likely aaWebApi endpoints on different servers although you could have aaWebApi reading multiple log directories from different servers.

arobinsongit commented 8 years ago

So I've been mulling this over a bit. I do agree with the general promise that clients should be able to maintain state on the client side so they can make intelligent calls.

Currently we have the following function public List<LogRecord> GetRecordsByStartMessageNumberAndCount(ulong MessageNumber, int Count)

that a client could use to accomplish the workflow you describe above. Also they could either specify a really large count (maximum integer) for messages to make sure they get all of the messages or they could batch until the returned count is less than than the specified count, which would tell them they are done.

I feel strongly that we should maintain the GetUnreadRecords functionality through the WebApi but we'll need to make sure we do a good job documenting the proper use with sending the client id through the header in the request.

Thoughts?

ryanvs commented 8 years ago

I like the simplicity of GetUnreadRecords and I think we should keep it. I have no problem if you want to merge the PR.

I do think we will need to enhance the information returned so the client can maintain state though. The simplest mechanism might be a integer hash code identifying the log file (basically override LogHeader.GetHashCode so it returns a repeatable, consistent, and unique value) and then either the log event id or file offset. From that information, aaLogReader can find the LogHeader and position of the client's last request. Perhaps we create a new method like GetUnreadRecordsWithState that returns a struct/class that combines all of the state info and returned log events.