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: add datadog back-end logging #1899

Merged
merged 5 commits into from
Aug 4, 2022
Merged

Conversation

halfwhole
Copy link
Collaborator

@halfwhole halfwhole commented Aug 1, 2022

Problem

Datadog currently does not receive any of our server logs

Solution

Added datadog-winston to send our server logs to Datadog. It works on staging, the staging server logs are visible on the log explorer

What would be a good name for service? Having something like [gov/edu/health]-[staging/production] seems right, but I also have some questions:

  • Should we add a prefix like go to the beginning (e.g. go-health-production)? The log explorer contains logs from all projects across OGP, so having a prefix can help to differentiate our project from other teams'
  • Our naming scheme on AWS is quite inconsistent, e.g. for the gov variant we have go-production on EB and gosg-production on RDS. Our team might want to discuss and standardise this?
  • Using assetVariant directly might not be the best idea, but this might depend on how we want to define the constants/env variables eventually (pending discussions in feat: datadog integration to client side using datadog/browser-rum lib #1896)

Deploy Notes

New dependencies:

  • datadog-winston: for sending winston logs to datadog

New dev dependencies:

  • @types/datadog-winston

@yong-jie
Copy link
Member

yong-jie commented Aug 1, 2022

naming scheme on AWS is quite inconsistent, e.g. for the gov variant we have go-production on EB and gosg-production on RDS

agreed. for a bit more context, Go's infra was originally gosg-*, same with elastic beanstalk as well. The reason why you're seeing it as go-* was because we needed to migrate our EB platform to Amazon Linux 2, and using a different prefix like go-* was the best differentiation strategy we could think of 😓

of course, we did not need to make any modifications to our RDS instance and therefore the name is still gosg-*

Should we add a prefix like go to the beginning (e.g. go-health-production

i vote yes for the same reason you mentioned!

@thanhdatle
Copy link
Contributor

thanhdatle commented Aug 2, 2022

agreed. for a bit more context, Go's infra was originally gosg-*, same with elastic beanstalk as well. The reason why you're seeing it as go-* was because we needed to migrate our EB platform to Amazon Linux 2, and using a different prefix like go-* was the best differentiation strategy we could think of 😓

This what we was guessing ytd

Should we add a prefix like go to the beginning (e.g. go-health-production

i vote yes for the same reason you mentioned!

It's a yes from me too!

@halfwhole
Copy link
Collaborator Author

halfwhole commented Aug 2, 2022

nice will go with the new service naming then! the env variable DD_SERVICE on gov/edu/health should be set to go-[gov/edu/health]-[staging/production], so should be able to use that by default instead of setting the service manually

have also updated the tracing utility to enable log injections, which helps datadog to connect our logs with our traces (https://docs.datadoghq.com/tracing/other_telemetry/connect_logs_and_traces/nodejs/)

@halfwhole halfwhole force-pushed the feat/datadog-server-logging branch from 791d250 to 862ae7a Compare August 3, 2022 02:28
Copy link
Member

@yong-jie yong-jie left a comment

Choose a reason for hiding this comment

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

nice work, code mostly lgtm. i dropped some suggested changes to improve code safety that we can discuss about!

src/server/config.ts Outdated Show resolved Hide resolved
@halfwhole halfwhole merged commit 8f5a429 into develop Aug 4, 2022
@halfwhole halfwhole deleted the feat/datadog-server-logging branch August 4, 2022 09:01
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