Fix for multiline errors when writing to log files

[jemrobinson]

✅ Checklist

• You have given your pull request a meaningful title (e.g. Enable foobar integration rather than 515 foobar).
• You are targeting the appropriate branch. If you're not certain which one this is, it should be develop.
• Your branch is up-to-date with the target branch (it probably was when you started, but it may have changed since then).

🚦 Depends on

n/a

⤴️ Summary

Fix how multiline errors are written to a log file

Previous

2024-06-20 15:07:29,837 - ERROR - Timezone 'Mars/OlympusMons' was not recognised.
'Mars/OlympusMons'

At c98ce56

2024-06-20 15:07:29,837 - ERROR - Timezone 'Mars/OlympusMons' was not recognised.
2024-06-20 15:07:29,837 - ERROR - 'Mars/OlympusMons'

At d08520d

2024-06-20 15:07:29,837 - ERROR - Timezone 'Mars/OlympusMons' was not recognised.
                                  'Mars/OlympusMons'

🌂 Related issues

n/a

🔬 Tests

Tested locally

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
data_safe_haven/administration/users
entra_users.py					69, 102, 117, 135, 151, 166
user_handler.py					71, 129, 142, 164, 211, 224
data_safe_haven/commands
shm.py					83, 107, 116
sre.py					106, 142, 151
users.py					54, 90, 158, 197, 266
data_safe_haven/context_infrastructure
infrastructure.py					83, 99
data_safe_haven/exceptions
__init__.py
data_safe_haven/external/api
azure_api.py					146, 186, 263, 300, 334, 376, 418, 453, 476, 521, 542, 582, 626, 654, 691, 729, 773, 830, 874, 926, 1007, 1039
graph_api.py					130, 167, 264, 312, 346, 382, 433, 465, 509, 521, 532, 557, 690, 754, 782, 810, 838, 867, 887, 910, 926, 947, 960, 1010, 1030, 1065, 1128
data_safe_haven/external/interface
azure_authenticator.py					67
azure_container_instance.py					89
azure_postgresql_database.py					113, 161
data_safe_haven/functions
strings.py
data_safe_haven/infrastructure
project_manager.py					143, 166, 199, 225, 235, 259-262, 292, 318, 328, 360, 373
data_safe_haven/infrastructure/components/dynamic
blob_container_acl.py					66, 86
entra_application.py					62, 113, 128, 159
file_share_file.py					97, 121
ssl_certificate.py					61, 143, 170
data_safe_haven/serialisers
yaml_serialisable_model.py
Project Total

This report was generated by python-coverage-comment-action

[JimMadge]

I'm not sure if either of these are right.

One is difficult to parse as some lines are log messages, some are continuations.
The other suggests there were two errors.

Maybe lines should be joined?

[jemrobinson]

What would your desired output look like?

[JimMadge]

What would your desired output look like?

Hmm, I'm not too sure. I'm not really familiar with log format conventions but I'm assuming trying to put multiline messages in a log might be frowned upon in the first place 😅.

I would think you'd want to keep to one message (even if it is long/multiline) per line in the logfile though.
Thinking you would want to be able to parse it like in journalctl to filter times, severity, etc.

[jemrobinson]

If you really want to emit structured logs, we should probably be giving JSON output...

[jemrobinson]

I'm wondering whether we ought to do something like this instead?

2024-06-20 15:07:29,837 - ERROR - Timezone 'Mars/OlympusMons' was not recognised.\n'Mars/OlympusMons'

possibly combined with replacing

msg = f"Something.\n{exc}"
raise Exception(msg) from exc

with

msg = f"Something"
raise Exception(msg) from exc

in our own code?

[JimMadge]

That's what I would have done as a first solution. Then see if very long lines become a big problem.

I suppose if would mean if you want to look through many lines you need an extra tool or bit of processing.

With tracebacks, I don't know if we need to print the previous exception in the new exception's message.

[jemrobinson]

Agreed - the insertion of newlines was done to ensure we got informative errors while skipping a full traceback. Note that other libraries (notably pydantic) also produce multiline errors, but we don't need to be doing it ourselves. I can update this PR next week - unless you want to do it as part of fixing logging?