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

Clean up log file export #50

Merged
merged 4 commits into from
Aug 13, 2024
Merged

Clean up log file export #50

merged 4 commits into from
Aug 13, 2024

Conversation

chroder
Copy link
Member

@chroder chroder commented Jul 18, 2024

There are two backwards incompatible changes here:

  • Removed LOGS_OUTPUT_FORMAT option -- only json everywhere by default. If you still wanted to use some other format, then you'd need to use custom vector config to send it and format it the way you want.
  • Changed the default LOGS_EXPORT_FILENAME to group similar log messages together to reduce number of exported files by default.

As such, we should bump the version.


Fixes:

  • A number of timestamps were not being parsed properly, leading to "ts" always being the processing time rather than the actual time that might exist in the real source log
  • Multi-line processing of PHP error log wasn't working properly

Exported Log Files

Added a new "log_group" property to each log transformer to organize messages into
logical groups. This is used in the new default LOGS_EXPORT_FILENAME format. This
change reduces the number of distinct files that get written to disk:

my-exported-logs/
  deskpro-app.log                # deskpro application logs (e.g. from PHP land)
  deskpro-tasks.log              # output from running cron
  deskpro-email_collect.log      # output from email collection service (if enabeld)
  deskpro-email_process.log      # output from email processing service (if enabeld)
  deskpro-svc_messenger.log      # output from svc_messenger service
  deskpro-svc_messenger_api.log  # output from svc_messenger_api service
  nginx-access.log               # nginx access log
  nginx-error.log                # nginx error log
  php-error.log                  # php error log
  php_fpm-error.log              # php fpm error log
  supervisor-procs.log           # supervisor output (and any low level output from php_fpm/nginx that might get raised on start)

@chroder chroder marked this pull request as draft July 18, 2024 14:32
- vector output logged to file and remove --quiet so there's actual useful output
- sets stdout_logfile_maxbytes on log files written by supervisor - they get rotated by our rotate-logs task already
- added a "FAST_SHUTDOWN" env var that'll set all stopwaitsecs=0 when enabled
- Updates vector to 0.39
- Removes logfmt option -- everything is json
- Fixes timestamp parsing on a number of log files where it was not working properly

*Exported Log Files*

Added a new "log_group" property to each log transformer to organize messages into
logical groups. This is used in the new LOGS_EXPORT_FILENAME format. This
change reduces the number of distinct files that get written to disk, making
it much easier to understand:

```
my-exported-logs/
  deskpro-app.log                # deskpro application logs (e.g. from PHP land)
  deskpro-tasks.log              # output from running cron
  deskpro-email_collect.log      # output from email collection service (if enabeld)
  deskpro-email_process.log      # output from email processing service (if enabeld)
  deskpro-svc_messenger.log      # output from svc_messenger service
  deskpro-svc_messenger_api.log  # output from svc_messenger_api service
  nginx-access.log               # nginx access log
  nginx-error.log                # nginx error log
  php-error.log                  # php error log
  php_fpm-error.log              # php fpm error log
  supervisor-procs.log           # supervisor output (and any low level output from php_fpm/nginx that might get raised on start)
```
@@ -99,10 +99,3 @@
it { should be_grouped_into 'root' }
it { should_not be_writable.by('others') }
end

describe file('/srv/deskpro/services/messenger-api/.env') do
Copy link
Member Author

@chroder chroder Jul 19, 2024

Choose a reason for hiding this comment

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

@MattGrundy FYI - this has been broken for a while I guess. I fixed it here by removing the test that checks for this - but please confirm it's expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like it may be a race condition, pulling a container locally and checking, the file gets created as expected. To double check, I checked out out main, added a fairly long sleep on the earthly run between starting the container and running and then the suite passes locally (it doesn't without the sleep).
I'll probably need to add is-ready type checks in like we do in our product suites and get these running on our PRs but I can pick that up separately so as to not block you.

@chroder chroder marked this pull request as ready for review July 19, 2024 11:05
@chroder
Copy link
Member Author

chroder commented Jul 19, 2024

@MattGrundy I did a bit more cleanup here... out of abundance of caution can you try running product CI on this before we merge.

And for the next onprem build we should bump the container version since this is technically a breaking change by removing logfmt, though I don't think anyone cares.

@chrislrobinson can you just confirm that changing the exported file names won't cause a problem for OPC?

Copy link
Contributor

@chrislrobinson chrislrobinson left a comment

Choose a reason for hiding this comment

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

LGTM. OPC doesn't rely on any of the log filenames anywhere (it just reads a list of files and displays them for download) so shouldn't have much of an impact there.

@MattGrundy MattGrundy merged commit b42872e into main Aug 13, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants