Improve Per Tenant Logging: Fix issues around default log file path

[shaangill025]

No description provided.

[shaangill025]

@WadeBarnes @esune The bdd tests are failing in Github actions but are passing locally with the same config [below]. It appears as if the changes to scripts like run_bdd and run_demo are not being applied.
LEDGER_URL=http://test.bcovrin.vonx.io PUBLIC_TAILS_URL=https://tails.vonx.io ./run_bdd -t @GHA

[WadeBarnes]

@shaangill025, According to the logs from the test run, the config is being picked up, but there is a permission issue writing to the logs. Search the logs for /home/aries/log and you'll see it.

It looks like the logs folder is being created by the Dockerfile.demo layer with slightly deferent permissions than the log folder at the Dockerfile layer.

https://github.com/hyperledger/aries-cloudagent-python/blob/8cfe8283ddb2a85e090ea1b8a916df2d78298ec0/docker/Dockerfile.demo#L25

https://github.com/hyperledger/aries-cloudagent-python/blob/8cfe8283ddb2a85e090ea1b8a916df2d78298ec0/docker/Dockerfile#L86-L87

I'm building the image and having a closer look at the permissions.

[WadeBarnes]

I think I see the issue. The run_bdd script for example, only creates the ../logs folder and correctly sets the permissions if the --timing flag is used, yet the ../logs folder is always mounted into the container, and the tenant logging expects the permissions to already be set properly. It's likely working locally for you because the ../logs folder already exists on your machine and is world writable.

https://github.com/hyperledger/aries-cloudagent-python/blob/8cfe8283ddb2a85e090ea1b8a916df2d78298ec0/demo/run_bdd#L75-L84

[WadeBarnes]

Might I also recommend cleaning up the whole logs vs log directory creation in the scripts and demo/bdd docker files to keep things consistent with the log directory created in the official docker file. That would help to avoid confusion.

[usingtechnology]

Going to add some comments for discussion.

My feeling is that we should NOT be automatically creating a log file. I think the deployment should specify a log file with a known location that the deployment can write. Pretty much what was in there before this. The only thing I would keep is the TimedRotatingFileMultiProcessHandler with multitenant support instead of the basic FileHandler. Forcing all deployments and running instances to write to a file is going to cause issues (already has). Trying to determine all the ways the code is deployed or run and set up the correct file location and permissions is not going to be possible. If we are to keep this default file logging then perhaps it should only throw warnings and acapy can start up and run if exceptions are raised while trying to create the log file.

Pretty sure the previous logging code would throw an exception if the specified --log-file couldn't be created. And the deployments would be corrected and provide a usable file location. I don't think we need to change that behaviour. And I don't think we should auto-create a log file - the deployment should specify that it wants/needs one and provides the correct location.

[WadeBarnes]

Has the per-tenant logging implementation introduced "always-on logging"? If so I agree with @usingtechnology. Though we still need to be consistent with what's already been defined for the log folder so there is a sane default location where the logs can be written without permission errors.

[usingtechnology]

Yes - by default acapy is trying to create a log file. I think we can 2 phase this.

1. In this PR remove the log file setting /home/aries/log/acapy-agent.log in all of the default configurations. Caller/Deployer must specify their log file (--log-file or their own --log-config).
2. In a separate PR standardize a good default location across all acapy images, including the test images. caller/deployer would still need to specify to create a log file via --log-file.

[WadeBarnes]

Yes - by default acapy is trying to create a log file.

Default logging should be to std-out like it's always been. Logging to a file should be an opt-in configuration process, such as when you're explicitly wanting to turn on pre-tenant logging.

[shaangill025]

@WadeBarnes I have done a cleanup on the log vs logs issue, but when I try to change the following:
https://github.com/hyperledger/aries-cloudagent-python/blob/4fa281a83491c9e5e6049baa9e49728127895df9/docker/Dockerfile.demo#L25

original: RUN mkdir demo logs && chown -R aries:aries demo logs && chmod -R ug+rw demo logs
updated: RUN mkdir -p demo log && chown -R aries:aries demo log && chmod -R ug+rw demo log

It errors out at chown -R aries:aries demo log, maybe because it has already been created with permissions [during ACA-Py startup]. Maybe this whole line can be commented out?

[WadeBarnes]

@WadeBarnes I have done a cleanup on the log vs logs issue, but when I try to change the following:

https://github.com/hyperledger/aries-cloudagent-python/blob/4fa281a83491c9e5e6049baa9e49728127895df9/docker/Dockerfile.demo#L25

original: RUN mkdir demo logs && chown -R aries:aries demo logs && chmod -R ug+rw demo logs
updated: RUN mkdir -p demo log && chown -R aries:aries demo log && chmod -R ug+rw demo log

It errors out at chown -R aries:aries demo log, maybe because it has already been created with permissions [during ACA-Py startup]. Maybe this whole line can be commented out?

Yes, that would likely be caused by the fact the log folder already exists in the base image.

https://github.com/hyperledger/aries-cloudagent-python/blob/4fa281a83491c9e5e6049baa9e49728127895df9/docker/Dockerfile#L78-L83

[usingtechnology]

I won't do the approval, I will wait on @WadeBarnes to weigh in...

I did run this through the ways I was having issues previously: locally through the devcontainer and locally from the plugins repo. Both run correctly with or without --log-file. 👍

I don't understand what is happening in run_demo and run_bdd. I did see the log directory when I did run_demo, but didn't see a log file created... run_bdd prompt seemed to look for a local log dir? Unclear. Someone (@WadeBarnes) will need to validate this works as expected.

[shaangill025]

I don't understand what is happening in run_demo and run_bdd. I did see the log directory when I did run_demo, but didn't see a log file created

I tried with ./run_demo performance --log-level debug --multitenant --log-file and ./run_demo faber --log-level debug --multitenant --log-file and it does create the log file.

With run_bdd, features file will have to be updated and --log-file added in there (done with commit 050eaf6).

[usingtechnology]

thanks @shaangill025 - ran the run_demo and works like a charm (with and without specifying log file). ok, good to know about the bdd - I could not see how logging was going to work there, glad it is a future enhancement.

good work!

[sonarqubecloud]

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud