Allow the process manager to configure log files count and sizes #1067

tahini · 2024-09-25T17:14:55Z

Use named objects instead of ordinal parameters for the numerous function parameters. This makes a call more readable, make it easier to fine-tune specific parameters while keeping the default values for others, and will make adding yet more parameters simpler.
Let ProcessManager support log file count and size as extra parameter
Let TrRoutingProcessManager support log file count and size as extra parameter. startBatch now also supports the debug option.
Add tests for the new functionalities

samuel-duhaime

Good.

tahini · 2024-09-25T20:01:50Z

I added the possibility to configure the log files and whether to run in debug in the config file by default, with always the possibility to override in individual calls, but we don't support it yet.

@greenscientist fwiw we can now allow more log files and of larger sizes in our instances and we could support running in debug in some instances if required, batch and not batch separately.

greenscientist · 2024-09-30T18:48:45Z

Kinda wondering if we should just strip all this out and let the standard OS tools handle log management.

greenscientist · 2024-09-30T18:49:34Z

@tahini, as-tu regardé si il existait des modules standard pour gérer tout ça ?

tahini · 2024-10-01T00:35:49Z

@tahini, as-tu regardé si il existait des modules standard pour gérer tout ça ?

C'est ce que winston fait, on peut envoyer les logs où on veut. Ceci a l'avantage que qqn qui fait rouler Transition peut contrôler le logging. Si on laisse l'OS faire, il faudrait documenter la procédure ou configurer un autre outil séparément.

packages/chaire-lib-backend/src/utils/processManagers/OSRMProcessManager.ts

greenscientist · 2024-10-01T17:55:38Z

packages/chaire-lib-backend/src/utils/processManagers/ProcessManager.ts

+ tagName,
+ serviceName,
+ nbFiles = 1,
+ maxFileSizeKb = 1000


The number here should be a constant

Also why use kilo instead if simply bytes.

The Kb is confusing, it should be KB if you want bytes, Kb would usually means bits

Also, a bit unreleated, but 1 meg is probably a bit small to be the default value. Thoughts?

It's the current default... fwiw, these values are to keep the current behavior, again fwiw...

Defaults to 3 files of 5MB each now, for the processManager and specifically for trRouting as well

greenscientist · 2024-10-01T18:00:06Z

packages/chaire-lib-backend/src/utils/processManagers/ProcessManager.ts

 cwd?: string;
 attemptRestart?: boolean;
+ logFiles?: {


Do we realely need a struct in a struct? This make calling the function a bit more complex with useful reason.

Debatable, but I'll say that one can leave it blank if they will, but if they want to set it, they'll want to handle the "log file" concept separately form the other 10 parameters that we set, so it might as well be a struct.

greenscientist · 2024-10-01T18:02:29Z

packages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.ts

@@ -14,6 +14,13 @@ import config from '../../config/server.config';
 // Set 2 threads as default, just in case we have more than one request being handled
 const DEFAULT_THREAD_COUNT = 2;

+type StartParameters = {


Please add a command to mention what is that type and where it will be used.

If it's of general use it should be in the ProcessManager file. Otherwise it whould be named TrRouting something...

Done, this is for the TrRotuingProcessManager only

greenscientist · 2024-10-01T18:05:15Z

packages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.ts

+ {
+ debug = false,
+ cacheDirectoryPath,
+ // Flag to enable the trRouting connection cache for all scenario


Is it possible to keep the comment after the param to make it clear to which entry it's related.

greenscientist · 2024-10-01T18:07:53Z

packages/chaire-lib-backend/src/utils/processManagers/TrRoutingProcessManager.ts

 const port = parameters.port || Preferences.get('trRouting.port');
 const cacheAllScenarios =
 config.trRoutingCacheAllScenarios === undefined ? false : config.trRoutingCacheAllScenarios;

 const params: Parameters<typeof startTrRoutingProcess>[3] = {
 debug: parameters.debug,
 cacheDirectoryPath: parameters.cacheDirectoryPath,
- cacheAllScenarios: cacheAllScenarios
+ cacheAllScenarios: cacheAllScenarios,
+ logFiles: parameters.logFiles


Not sure it's a good idea of expose the logFiles setting externally. We should know what we expect here.

Another option for this PR would be to simply put the winston config in the config.js file (maybe not exactly the config, but something tha twill allow us to easily build that config). That would avoid all the parameter passing, each service can get its log configuration and we won't have to add too much when we want to support something else than log files.

I updated so log files are only configurable through the config, there's no use case for individually setting it.

greenscientist · 2024-10-01T18:12:25Z

examples/config.js

@@ -17,6 +17,19 @@ module.exports = {

 // Enable caching of connections for all scenarios in trRouting. Will use more memory
 trRoutingCacheAllScenarios: false,
+ // Extra options for running trRouting


We if add a config block for trRouting, then we need to move the cacheAllFlag inside of it.

We should also move the port number there.

Done in previous PR

Go from 3.7.2 to 3.14.2

The `startProcess` function receives 8 arguments as parameter, that's a lot and difficult to read when calling the function. Adding more arguments will only get more difficult. So we switch to named arguments instead.

Add an argument `logFiles` in the ProcessManager's `startProcess` function. This argument takes the maximum size in kilobytes of the log files and the maximum number of desired files.

…eparately The `TrRoutingStartParameterss` type contains all the extra parameter of the various start functions, so this type can be re-used in all functions (`start`, `restart` and `startBatch`). All but the first argument of the `startBatch` function is now part of a unique object with named arguments, adding the `debug` parameters.

tahini · 2024-10-10T19:25:44Z

@greenscientist I rebase and updated to address all your comments. With this, next time we need to investigate some trRouting related errors, we'll be ready to configure debug and logging in our instances.

greenscientist

One last question.

greenscientist · 2024-10-11T13:26:33Z

packages/chaire-lib-backend/src/utils/processManagers/ProcessManager.ts

@@ -12,6 +12,8 @@ import serviceLocator from 'chaire-lib-common/lib/utils/ServiceLocator';
 import ProcessUtils from './ProcessUtils';

 const PID_DIRECTORY = 'pids';
+const DEFAULT_LOG_FILE_COUNT = 3; // 3 files


Shouldn't those be removed in the last commit, now that we have default in the config file???

The config is only for trRouting, this is the main process manager and applies to all possible processes to start.

Can we combine those constant somehow? Can the trRouting default config information use those constant ?

Moved to the config file

This adds a `debug` and `logs` configuration option to the TrRoutingConfig type. They can be configured on the `config.js` used in the project. `debug` is a boolean option specifying whether to start the trRouting process in debug mode `logs` configures the loggins of the trRouting process. Currently, the options that can be set are `maxFileSizeKB` for the maximum size of a log file in kilobytes and `nbFiles` to specify the number of log files to keep. They can be set separately for the batch or single instance. These onfigurations are optional. Move the default log file count and size to the config instead of the `ProcessManager.ts` file.

tahini requested review from greenscientist and samuel-duhaime September 25, 2024 17:14

samuel-duhaime approved these changes Sep 25, 2024

View reviewed changes

greenscientist requested changes Oct 1, 2024

View reviewed changes

tahini mentioned this pull request Oct 3, 2024

trRouting: Move port and cacheAllScenarios to instance specific config #1072

Merged

tahini added 3 commits October 10, 2024 12:00

dep: Upgrade version of the winston package

7ee8ada

Go from 3.7.2 to 3.14.2

processManager: Use named object arguments instead of ordinal

2129d13

The `startProcess` function receives 8 arguments as parameter, that's a lot and difficult to read when calling the function. Adding more arguments will only get more difficult. So we switch to named arguments instead.

processManager: Support specifying log file size and count

45732be

Add an argument `logFiles` in the ProcessManager's `startProcess` function. This argument takes the maximum size in kilobytes of the log files and the maximum number of desired files.

tahini force-pushed the processManagerLogFiles branch from 406414f to 15298ef Compare October 10, 2024 19:17

tahini added 2 commits October 10, 2024 15:19

TrRoutingProcessManager: Add unit tests for the restart function

af53b5e

tahini force-pushed the processManagerLogFiles branch from 15298ef to bd6b6c5 Compare October 10, 2024 19:19

greenscientist reviewed Oct 11, 2024

View reviewed changes

tahini force-pushed the processManagerLogFiles branch from bd6b6c5 to 208d331 Compare October 11, 2024 15:49

greenscientist self-requested a review October 11, 2024 16:24

greenscientist approved these changes Oct 11, 2024

View reviewed changes

tahini merged commit ed91875 into chairemobilite:main Oct 11, 2024
6 checks passed

tahini deleted the processManagerLogFiles branch October 11, 2024 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the process manager to configure log files count and sizes #1067

Allow the process manager to configure log files count and sizes #1067

tahini commented Sep 25, 2024

samuel-duhaime left a comment

tahini commented Sep 25, 2024

greenscientist commented Sep 30, 2024

greenscientist commented Sep 30, 2024

tahini commented Oct 1, 2024

greenscientist Oct 1, 2024

greenscientist Oct 1, 2024

greenscientist Oct 1, 2024

tahini Oct 1, 2024

tahini Oct 10, 2024

greenscientist Oct 1, 2024

tahini Oct 10, 2024

greenscientist Oct 1, 2024

greenscientist Oct 1, 2024

tahini Oct 10, 2024

greenscientist Oct 1, 2024

greenscientist Oct 1, 2024

tahini Oct 1, 2024

tahini Oct 10, 2024

greenscientist Oct 1, 2024

greenscientist Oct 1, 2024

tahini Oct 10, 2024

tahini commented Oct 10, 2024

greenscientist left a comment

greenscientist Oct 11, 2024

tahini Oct 11, 2024

greenscientist Oct 11, 2024

tahini Oct 11, 2024

Allow the process manager to configure log files count and sizes #1067

Allow the process manager to configure log files count and sizes #1067

Conversation

tahini commented Sep 25, 2024

samuel-duhaime left a comment

Choose a reason for hiding this comment

tahini commented Sep 25, 2024

greenscientist commented Sep 30, 2024

greenscientist commented Sep 30, 2024

tahini commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tahini commented Oct 10, 2024

greenscientist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment