✨Adds timestamps to sidecars (dynamic and computational) #2817
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2817 +/- ##
========================================
- Coverage 78.9% 78.9% -0.1%
========================================
Files 671 671
Lines 27290 27296 +6
Branches 3148 3148
========================================
- Hits 21542 21541 -1
- Misses 4992 4996 +4
- Partials 756 759 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| for message in log_msg: | ||
| await self._channel_queues[self.CHANNEL_LOG].put(message) | ||
| with_timestamp = f"{time_string} {message}" |
There was a problem hiding this comment.
THOUGHT:
When you add something in a log, i would always make sure it can be parsed back easily and reliably. e.g. the front end might want to add that in an extra column. Your test below clearly suggests otherwise
For instance #2157 we show some ideas on how to achieve that.
There was a problem hiding this comment.
@pcrespov where you trying to add an example above?
There was a problem hiding this comment.
My idea here was to avoid a mess in the UI and make it as minimalistic as possible.
| def mock_datetime_utc_now( | ||
| monkeypatch: MonkeyPatch, | ||
| ) -> datetime.date: | ||
| # Note: since datetime is unmutable, best approach is to |
There was a problem hiding this comment.
Are you sure about this? Are you by any chance trying to datatime instead of datetime?
Check this
https://stackoverflow.com/questions/43799206/python-how-do-i-mock-datetime-utcnow
There was a problem hiding this comment.
So I finally managed to patch the object with something inspire form https://stackoverflow.com/a/51213128/2855718
You need to wrap the class in a mock and than you can access it's methods.
But I got stopped by tenacity, which apparently uses datetime to compute some metric.
Since I don't want to spend to much time on it. this current implementation is the best I have.
There was a problem hiding this comment.
As discussed, I think that the log mechanism needs to be revisited (It would be good to get @sanderegg input on that as well) but if this fixes the current needs, go ahead.
| @@ -168,8 +174,10 @@ async def post_log_message(self, log_msg: Union[str, List[str]]) -> None: | |||
| if isinstance(log_msg, str): | |||
| log_msg = [log_msg] | |||
|
|
|||
| time_string = _get_utc_now().strftime(LOG_DATETIME_FORMAT) | |||
There was a problem hiding this comment.
so if I get it correctly you actually add the timestamp when logging to rabbitMQ right?
in the dask-sidecar I already get the timestamps from the docker container directly, see here if possible which I think you can.
I am missing a timestamp for messages not from the container, that is right, but I can see that if we need to debug it is probably easier if the timestamp is the same as what we would see in graylog.
Shall we go for adding a timestamp only if it is missing? or having a rabbitMQ timestamp and a container timestamp? I mean the message in rabbitMQ is pretty flexible. I think we can pass a json with several fields. what do you think?
There was a problem hiding this comment.
I agree with fetching the timestamps from the container and injecting it where missing.
Currently the timestamp is part of the message, as also it is part of the message the source.
If you'd like to inject other files, at this point we might want to transform the LoggerRabbitMessage from sending a list of strings containing the messages to the logger to sending a json. At that point the fronted will format the logs.
Is something like this what you had in mind? Not sure if doing this would push too much work on the frontend.
|
outdated, issue was already closed |
What do these changes do?
To allow users to more easily figure out what is happening with the services, timestamps were added to the logs coming from sidecars.
Related issue/s
How to test
Checklist