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

Disable buffered logs #3892

Merged
merged 6 commits into from
Feb 13, 2024
Merged

Disable buffered logs #3892

merged 6 commits into from
Feb 13, 2024

Conversation

brody192
Copy link
Contributor

@brody192 brody192 commented Feb 9, 2024

Hosts like Railway use line scanners and thus when logs are buffered the result is that no logs show up.

Copy link

github-actions bot commented Feb 9, 2024

Warnings
⚠️ Changes were made to the environment variables, but not to the documentation - Please review your changes and check if a change needs to be documented!

CLA

Hello there and welcome to our project!
By submitting your Pull Request, you acknowledge that you agree with the terms of our Contributor License Agreement.
Although we don't have a dedicated legal counsel, having this kind of agreement can protect us from potential legal issues or patent trolls.
Thank you for your understanding.

Generated by 🚫 dangerJS against 3a3fb9f

@@ -16,7 +16,7 @@ import { EnvironmentService } from './integrations/environment/environment.servi
const bootstrap = async () => {
const app = await NestFactory.create(AppModule, {
cors: true,
bufferLogs: true,
bufferLogs: false,
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with this change. @magrinj Is this useful to have buffered logs, if yes, maybe we should introduce an environment variable to leave to the user to choose what they prefers

@charlesBochet
Copy link
Member

@brody192 I've actually looked into this a bit more deeply and I am not sure to understand the problem.
The bufferLog should only delay the logs to wait for the nestjs app to be loaded but not change their format. Why is it an issue on railways?

@brody192
Copy link
Contributor Author

brody192 commented Feb 9, 2024

in my testing it just ends up looking like there are no logs coming through, even when ran for an hour, when logs are being buffered instead of immediately flushed. there are no lines for railway to scan so nothing gets sent to the deploy logs

@charlesBochet
Copy link
Member

can you try again with the latest version of main?
Make sure that you have the following env variables setup:
LOG_LEVELS=error,warn,log
LOGGER_DRIVER=console

It should work, I don't think it is specific to Railways

@brody192
Copy link
Contributor Author

brody192 commented Feb 9, 2024

looks like log buffering is still enabled on the main branch, it's not going to work if logs aren't flushed on every log write, otherwise there are no logs for railways logging infrastructure to scan over, unless setting it to console mode disables buffering?

I've tested twenty with and without log buffering enabled, logs only show when buffering is disabled.

will try those variables!

though, buffered logs are the number one reason we see users ask why they can't see their app's logs in railway.

@charlesBochet
Copy link
Member

Interesting! I have no deep experience in this part, so I'm going to ask naive questions :p

I'm having trouble to understand how the fact NestJS log buffering is working locally but not on with railway?
What makes it works in local on my computer but not on railway containers/server?

Here is my local console output when I launch twenty-server on my computer:
image

@brody192
Copy link
Contributor Author

brody192 commented Feb 9, 2024

on railway they use a line scanner to parse each log line, but if logs are in a buffer that doesn't work, it only works when each log line is flushed individually, your local terminal has no issues showing you the log buffer.

@charlesBochet
Copy link
Member

@brody192 Ok, could we add a LOGGER_IS_BUFFER_ENABLED env variable, default true, optional

  • in .env.example
  • in doc (self hosting > env variables)
  • in environment.service.ts
  • use it in main.ts, command.ts, queue-worker.ts

@brody192
Copy link
Contributor Author

I do not see any benefits to having log buffering enabled anyway. please share your reasoning for wanting it enabled (by default)

@charlesBochet
Copy link
Member

charlesBochet commented Feb 12, 2024

@brody192 If you don't enable it, it seems that NestJS is not sending the startup logs through the logger
https://docs.nestjs.com/techniques/logger
In the example above, we set the bufferLogs to true to make sure all logs will be buffered until a custom logger is attached (MyLogger in this case) and the application initialisation process either completes or fails. If the initialisation process fails, Nest will fallback to the original ConsoleLogger to print out any reported error messages. Also, you can set the autoFlushLogs to false (default true) to manually flush logs (using the Logger#flush() method).

I feel the default behavior should be that all logs go to the desired logger (if you setup a S3 logger, you also want the startup logs to go there)

I think as long as it can be enabled through env vars, so it also works with railway scanner, everybody should be happy!

@charlesBochet
Copy link
Member

@brody192 I'll go for this approach to unblock Railway users. Beside logs, did you manage to deploy Twenty on Railway?

@charlesBochet charlesBochet merged commit 52bb33b into twentyhq:main Feb 13, 2024
12 checks passed
@brody192
Copy link
Contributor Author

Sorry, I just saw this now!

thank you for the detailed explanation, I was only thinking in the context of hosting the app on Railway or similar service, that all makes perfect sense now!

the variables you provided do get the logs coming through, albeit all at once, but that's good enough for now!

LOG_LEVELS=error,warn,log
LOGGER_DRIVER=console

and yes aside from the logs, I did manage to get it working wonderfully on Railway, they have a template concept that aims to offer near one-click deploys for sample apps and even as much as full-stack self-hostable apps like Twenty, here's the template I made https://railway.app/template/to7duB please check it out!

@charlesBochet
Copy link
Member

This is great!
@FelixMalfait FYI, we shoud definitely add this to the self hosting guide so people can easily self host on Railway!

@brody192 brody192 deleted the patch-1 branch February 13, 2024 19:32
@brody192
Copy link
Contributor Author

self-hosting with my template would be as easy as clicking deploy now, saving the configs, filling out the SMTP stuff, and then clicking the final deploy button, wait a bit and you have a ready-to-use install of twenty!

@FelixMalfait
Copy link
Member

@brody192 that'd be amazing. I had tried to deploy to Railway but I didn't manage to do it because we rely on pg_graphql and they didn't support this Postgres extension / deployment of a custom Postgres through Docker.
Long-term we might have to find an alternative to make it easier for people to deploy, but right now it's a core piece of our backend and Twenty cannot work without this extension

@brody192
Copy link
Contributor Author

Railway supports docker images now and everything in the template deploys from a twenty docker image including the database!

@FelixMalfait
Copy link
Member

Oh great!

@brody192
Copy link
Contributor Author

I've now added LOGGER_IS_BUFFER_ENABLED set to false on the template, thanks for everything!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants