Skip to content
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

Logging refactor #504

Draft
wants to merge 60 commits into
base: main
Choose a base branch
from
Draft

Logging refactor #504

wants to merge 60 commits into from

Conversation

F1F7Y
Copy link
Member

@F1F7Y F1F7Y commented Jul 7, 2023

Refactor to logging to make it more readable ( eg currently you can use both spdlog::info and NS::log::NS->info, this is bad )

Rough list of things:

  • Refactor logging functions
  • More logging categories
  • Separate log files into a folder
    • Current idea is: ( Very much up to discussion )
      └── %Y-%m-%d_%H-%M-%S/
        ├── native.txt
        ├── warning.txt
        ├── error.txt
        ├── script.txt
        ├── script_warning.txt
        ├── crash.txt
        └── minidump.dmp
      
- ~~[ ] Print system and northstar information~~
- ~~[ ] Ability to toggle ( most likely through a launch arg ) what is logged into gameconsole~~
- [x] `-wconsole` arg to disable windows terminal
- [x] Fix crashlogs not flushing to file

Depends on ( and should preferably be shipped with ): #477 

@F1F7Y F1F7Y marked this pull request as draft July 7, 2023 14:12
@F1F7Y F1F7Y added the depends on another PR Blocked until the PR it depends on is merged label Jul 7, 2023
@GeckoEidechse
Copy link
Member

( eg currently you can use both spdlog::info and NS::log::NS->info, this is bad )

Yup, glad to see a change on that 👀

Separate log files into a folder

  • Current idea is:
    ( Very much up to discussion )

I would keep the logs in a single file. Given the log prefix one can still strip out certain parts by e.g. using grep. Splitting it up into different files means that when a helper requests a user to upload current logs, the user then has to zip up the entire folder and then upload that. This might not seem like much effort but if you ever interacted with tickets you know that this will be problematic xD

@F1F7Y
Copy link
Member Author

F1F7Y commented Jul 7, 2023

I would keep the logs in a single file. Given the log prefix one can still strip out certain parts by e.g. using grep.

The main reason i want to wrap them in a folder is separating the crash message into a separate file. Perhaps it could even be a json so that other apps could parse it

Splitting it up into different files means that when a helper requests a user to upload current logs, the user then has to zip up the entire folder and then upload that. This might not seem like much effort but if you ever interacted with tickets you know that this will be problematic xD

I can see some users really struggling with this. Even with improving error handling ( script errors, ... ) this would still be a problem.

@GeckoEidechse
Copy link
Member

I would keep the logs in a single file. Given the log prefix one can still strip out certain parts by e.g. using grep.

The main reason i want to wrap them in a folder is separating the crash message into a separate file. Perhaps it could even be a json so that other apps could parse it

If the crash is caused by a mod, then having information about which mods are loaded in the same file would be quite useful in my eyes ^^

I assume whether it's stored in one file or multiple can be easily adjusted by just modifying the code related to the logger backend? In which case switching between the two should be rather simple once we decide which mode to choose (I'd still argue for single file :P)

@F1F7Y
Copy link
Member Author

F1F7Y commented Jul 7, 2023

I assume whether it's stored in one file or multiple can be easily adjusted by just modifying the code related to the logger backend? In which case switching between the two should be rather simple once we decide which mode to choose (I'd still argue for single file :P)

spdlog makes it really easy to switch between the configurations, so for the sake of testing i could implement it so that we know which one has more pros

pg9182
pg9182 previously requested changes Jul 8, 2023
NorthstarDLL/CMakeLists.txt Outdated Show resolved Hide resolved
@F1F7Y
Copy link
Member Author

F1F7Y commented Jul 13, 2023

I'd say this is pretty much done, just waiting on #477

@GeckoEidechse GeckoEidechse dismissed pg9182’s stale review July 28, 2023 17:30

Requested changes were addressed

@ASpoonPlaysGames ASpoonPlaysGames added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Sep 2, 2023
}

SQObject functionobj {};
int result = sq_getfunction(m_pSQVM->sqvm, funcname, &functionobj, 0);
if (result != 0) // This func returns 0 on success for some reason
{
NS::log::squirrel_logger<context>()->error("Call was unable to find function with name '{}'. Is it global?", funcname);
Error(SQ_GetLogContextNative(context), NO_ERROR, "Call was unable to find function with name '%s'. Is it global?\n", funcname);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes #485

@github-actions github-actions bot added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Dec 20, 2023
@GeckoEidechse
Copy link
Member

@F1F7Y am I correct assuming assuming that this is basically the same as #617 ?

@GeckoEidechse GeckoEidechse removed the depends on another PR Blocked until the PR it depends on is merged label Dec 21, 2023
@GeckoEidechse
Copy link
Member

(Also I just noticed that the label for dependent PR was never removed when #477 was merged causing this PR to be stuck in PR hell, ugh. We really need some way to automate this as otherwise it will become impossible to keep track of stuff in the long run)

@F1F7Y
Copy link
Member Author

F1F7Y commented Dec 21, 2023

@F1F7Y am I correct assuming assuming that this is basically the same as #617 ?

Unless i missed some change it should be yeah.

@GeckoEidechse GeckoEidechse added the primedev This PR comes from cherry-picking primedev label Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflicts Blocked by merge conflicts, waiting on the author to resolve needs code review Changes from PR still need to be reviewed in code needs testing Changes from the PR still need to be tested primedev This PR comes from cherry-picking primedev
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants