-
Notifications
You must be signed in to change notification settings - Fork 0
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
Hookup for log/event messages to history server #218
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
dfa23ec
to
5172e14
Compare
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.
LGTM 👍
Just some comments/suggestions.
if data := device.get(): | ||
if isinstance(data, dict): | ||
for vial, output in data.items(): | ||
self.history.put(name, "sensor", output, vial=vial) | ||
else: | ||
self.history.put(name, "sensor", data, vial=getattr(data, "vial", None)) |
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.
[comment] I can see this being duplicated if logging is needed from somewhere else. It would be better if self.history.put
could handle this.
@@ -8,6 +8,8 @@ | |||
|
|||
class HistoricDatum(BaseModel): | |||
timestamp: float | |||
kind: str |
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.
[comment] This would benefit from having a description.
vial = getattr(record, "vial", None) | ||
record_dict = {"level": record.levelname, "message": record.getMessage()} | ||
# TODO: add support for some extra fields (but these should be json-seralizable) | ||
kind = "event" if record.levelno == EVENT else "log" |
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.
[comment] It might help the TODO implementer if we added here and now, how we might envision coding this, e.g., using a match
, an enum
, a mapping etc?
evolver/base.py
Outdated
@@ -312,7 +312,7 @@ def init_descriptors(self, **non_config_kwargs): | |||
init_and_set_vars_from_descriptors(self, **non_config_kwargs) | |||
|
|||
def _setup_logger(self): | |||
self.logger = logging.getLogger(self.name) | |||
self.logger = logging.getLogger(f"{__package__}.{self.name}") |
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.
[comment] It might be slightly more robust against future changes if we added a setting for this rather than relying on the convention of it always being __package__
.
E.g.:
# evolver.settings
class Settings(...):
ROOT_LOGGER_NAME: str = __package__
6f82ee9
to
13f2c45
Compare
Thanks. I made some corresponding changes, I think scope of them is small enough I will consider approval sticky and merge this in 😄 |
handling reshape of history data, related to ssec-jhu/evolver-ng#218
To utilize the existing history server for logs in addition to sensor data, the following steps were taken:
Resolves: #131, Resolves #196
Enables/relates to: #109, #132
Some examples: