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

Feat remove google logger #326

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bneigher
Copy link
Contributor

No description provided.

@simenandre
Copy link
Member

simenandre commented Sep 27, 2023

Sorry for being unclear. I don't think we should remove it entirely, but make it a peer dependency.

What do you think about it? There are users who are pushing this to the Google Cloud logger, and picking up with tools liks gcl-slack. But again, having used this setup for many years, I'm no longer sure that is a good approach. Very curious what you think we should do.

@bneigher
Copy link
Contributor Author

@simenandre I think a good design approach for this would be for the application to instantiate the google logger and set nestjs to use that logger. by doing that, this module will use the google logger. this module shouldn't need google libraries as a dependency or a peer dependency IMO.. separate concerns

@bneigher
Copy link
Contributor Author

but I understand not wanting to make this change... potentially a major version change in this case if you want to proceed with this approach

@simenandre
Copy link
Member

It's fine for me that we break stuff, as long as it's easy to migrate 👍

Google Cloud Logger isn't really used for logging in our case, it's used to push a Slack object to a log. We could use the generic NestJS logger for this, or even console.log as long as we are sending JSON the way Google Cloud logger wants.

The design behind using Google Cloud is to use the log as a transport for Slack, removing requests for the application layer. Another application picks up this log message and sends it to Slack, i.e. a Cloud Function or otherwise. Could be done the same way in AWS, Azure and Kubernetes, etc. However, I'm not sure about this design and if it's worth maintaining.

@bneigher
Copy link
Contributor Author

Ah I see. That makes sense. Well I'll leave the decision up to you.. feel free to close the MR if you want to not proceed.

It probably can be configured to work with @google logger as a peerDependenecy but I'm afraid my @nestjs module development skills are not quite there to make the module's logger injection be optional

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.

2 participants