Logging for SmartRedis #281
Conversation
billschereriii
added
type: feature
|
Initial C++ implementation is code complete, but not tested (or even compiled!). Provided for team review of implementation direction only |
Assorted fixes [ Committed by @ashao ] [ Reviewed by @billschereriii ]
Merge branch 'lager' of github.com:billschereriii/SmartRedis into lager
billschereriii
requested review from
mellis13,
al-rigazzi,
ashao and
MattToast
There was a problem hiding this comment.
Really great implementation and you did a great job keeping it very compact. I really appreciate the refactor to isolate the places where we get the values of configuration variables.
The largest comment/criticism that I have is that the logging is quite closely tied to the Client. As we've discussed offline, this might interfere with multiple client support down the line, but after reviewing this PR there's a larger question about how the logging interacts with various entities within SmartRedis. My suggestion is that we pass in the SmartRedis object that we're trying to log and that the identifier that appears in the logging file is extracted from the object. For example:
log_message(dataset, "Corralling llamas")
log_message(client, "Reticulating splines")
with the resulting logfile saying
dataset-1: Corralling llamas
client-foo: Reticulating splines
In this way, the Client, Dataset, and Logger are all first-class entities (which interact with each other) whereas now the Logger is somewhat subordinate to Client. This can be seen most clearly in the Dataset constructor which allows you to set a client id. Happy to talk about this in greater detail.
UPDATE:
After talking with Bill, my mental model of the above is incorrect, arising from my assumption that client_id referenced in the logger was directly tied to our SmartRedis client. Changing that from client_id to logger_id makes it easier to distinguish.
Another question to be discussed: Should we have the logger put the name of the Dataset or Client in the logging file.
There was a problem hiding this comment.
Great PR! This is a huge step in the right direction for the code. I had similar general questions as Andrew regarding code structure, but won't repeat here. I'd be happy to join follow-up design discussions.
Just as a final note, we should run the scaling tests before merging to make sure there is no perf hit.
There was a problem hiding this comment.
PR looks amazing!! I took a look mostly at the python side of things to make sure my feedback was documented were it was most useful. I have a couple of things I wanted to confirm but otherwise looks fantastic!
I'll circle back around tomorrow to look over the rest of it in more detail tomorrow, but scanning over Andrew and Matt's feedback it looks like they have already commented on, and you have addressed, most of my concerns
| #include "srexception.h" | ||
| #include "logger.h" | ||
| //#include "srobject.h" |
There was a problem hiding this comment.
Is there a reason these comments were left?
There was a problem hiding this comment.
I'll clean this up as part of ticket #338. It doesn't really hurt anything before then.
| from .smartredisPy import PyLogContext | ||
| from .util import exception_handler, typecheck | ||
|
|
||
| from .error import * |
| from .smartredisPy import PySRObject | ||
| from .util import exception_handler, typecheck | ||
|
|
||
| from .error import * |
| :type name: str | ||
| """ | ||
| typecheck(context, "context", str) | ||
| self._name = context |
There was a problem hiding this comment.
Storing the ctx in a "protected" variable, but doesn't seem to be used internally within the class. Could be problematic if an app dev were to instance and then call set_data. Maybe remove or replace with
@property
@exception_handler
def name(self):
# Note: This would require adding new bindings
self._data.get().get_context()There was a problem hiding this comment.
Object-based context in Python isn't working yet, but will be resolved shortly as part of #338
| :type name: str | ||
| """ | ||
| typecheck(context, "context", str) | ||
| self._name = context |
There was a problem hiding this comment.
Similar ctx is stored in a "protected" member and not used internally as noted on the python SRObject class. Maybe remove or make a property?
There was a problem hiding this comment.
Object-based context in Python isn't working yet, but will be resolved shortly as part of #338
There was a problem hiding this comment.
But the property approach looks completely viable
No description provided.