-
Notifications
You must be signed in to change notification settings - Fork 687
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
fix: cleanup log output #345
Conversation
@marionbarker would you mind running your scripts to see the diff in log size? Thanks! |
I applied this patch about a day and a half ago to latest Trio - dev (at the time).
From this comment, the prior art was about 3.3k lines/hour, 288k characters/hour. The graphic is the NS site used with this test phone. |
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.
Giving my approval based on the fact it works for providing record of Omnipod messages.
Someone else more familiar with these logs should examine the result for other content.
I recommend a Squash & Merge. My history is a lot of trial and error. |
I will have a look at the logs tonight |
@mkellerman The content pf the logging is totally readable and fine with me. I suggest that you give the RED marked part another context e.g. like oref, but not middleware. Edit: changed green to RED, that was a mistake by me |
@mountrcg, i'm unable to reproduce the autosens logs like you. Mine get trimmed correctly. Maybe something to do with Timezone/Language formatting? Regarding 'Middleware', this is autogenerated by how we call the JavaScriptWorker... I didn't name these individually. |
@marionbarker @mountrcg posted a new fix that might resolve the issue. |
Summary:Log files and time stamps are provided for someone else to review. Configuration:
Medtronic Log:
Omnipod DASH Log:
The Omnipod messages are not affected by the logging changes, the parser still works for code both with and without PR 346 applied. |
Thanks @marionbarker ! I dont get any of those u(xxxx) in my logs, so it's really hard to debug this one on my own. But i did fix the regex to include this pattern. it should be fixed now. |
Downloaded new patch and built to
Both phones are also running changes from PR 353. Below are the log files (so since midnight local on 2024-07-19) acquired around 05:50 am MDT: Pod:
|
Not seeing much improvement. Still seeing Middleware a lot and I think that's what was making @mountrcg unhappy. Uploading the log file and the patch I used in case I didn't do the modification correctly. iPhone 8 running iOS 16.7.8 I rebuilt a couple of times this morning. The final time was with the patch applied (for sure). Using an rPi DASH simulator as a pump. The Omnipod Message parser still works as expected.
log file: |
@marionbarker I think i found the line that uses 'Middleware' that might be affecting you (as i dont see it in my logs). Let me know if that resolves it for you now. (no rush, clearly we are focusing on the RC). |
Configuration
The Omnipod parser code works fine with this log file. Trim the log file to start when new build was started. |
Dear @mkellerman I looked at the log output with the current changes. As shown by you further up it is like this:
compared to before the changes in this PR
This does not add any meaningful context, it just increase the logging size by adding the Middleware here has nothing to do with the log output determine_basal.js creates. middleware as in ![]() only injects certain values into determine_basal.js but does not affect the logging output of those lines you marked at all. Therefore I think those lines should not be marked with
which is already marked and searchable. I would suggest to skip your middleware marking, without knowing how you do your magic :-) |
@mountrcg , I am unsure if @mkellerman knows what middleware is and how it is used in the context of Trio? If so, perhaps you could explain and give an example? |
I'm aware of what middleware does, but I'm printing the script that is executed. I'm not naming things myself. Please look at the code change to see how this value is evaluated. In my opinion, if we don't want middleware/*.js, we should change the name of the file. The point of the log is to show you what file ran and what console output was provided. Which shouldn't be part of this fix. This is only to cleanup the autosens logs. |
Hi @mkellerman , @dnzxy and myself looked at again. Still does the middleware thing via file structure on oref output, as middleware wraps around the oref call to set values that normally be calculated by oref or be profile.settings.
|
Sorry for the slow response. Good idea on using only the filename, and not the folder. I'm doing a fix now. Should be up for review shortly! |
Does this work for everyone?
|
|
Any other suggestions? Would love to get this one merged in. |
Personally happy with this. Approving. |
StatusSuccess Configuration
ResultsOmnipod parser still works with modified logs. Log Sizes:
a few side-by-side screen shots. Log file acquired at 2024-10-13 20:25
|
Here is an additional log with only this build. The phone was locked overnight until just before sharing the log. The log size (2.7 MB) for about 6.5 hours is still in the range of 0.42 MB/hr. |
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, output is way cleaner. And the code and regex are concise. Good work @mkellerman !
Finally we are done with the bloating logs from JS
Fix for #272
prepare/autosens.js
,Middleware
)Reference to previous PR #331
New output: