feat: datadog integration to client side using datadog/browser-rum lib #1896
Conversation
thanhdatle
changed the title
| - run: | | ||
| if [[ $GITHUB_REF == $STAGING_BRANCH ]]; then | ||
| echo SERVERLESS_STAGE=staging >> $GITHUB_ENV; | ||
| elif [[ $GITHUB_REF == $PRODUCTION_BRANCH ]]; then | ||
| echo SERVERLESS_STAGE=production >> $GITHUB_ENV; | ||
| fi |
There was a problem hiding this comment.
It looks like these file-changes were meant to fix a serverless deployment bug, and is out of scope of this PR.
As discussed in our agreement with squash-merging, doing multiple things within a PR can be dangerous with squash-merging, because fine-grained code changes can no longer be rolled back independently. Hypothetically if we needed to roll back the serverless fixes for further work, we would need to roll back your datadog integration code too (vice versa).
As such, could i trouble you to move these changes to a separate PR? thanks!
There was a problem hiding this comment.
I did this because I need the SERVERLESS_STAGE in the docker build command as a build argument.
There was a problem hiding this comment.
makes sense. just a heads up, i'm seeing another PR that will remove the SERVERLESS_STAGE env variable in favor of BRANCH_ENV, which might be an incoming merge conflict.
.eslintrc.json
Outdated
| "globals": { | ||
| "DD_SERVICE": true, | ||
| "DD_ENV": true | ||
| }, |
There was a problem hiding this comment.
This declaration could be misleading (to both engineers, and ESLINT), because these variables only exist in the frontend, and aren't truly global
Relating to a previous comment, if DD_ENV and DD_SERVICE were injected into process.env instead of globally, we would not need to introduce global variables (bad coding practice) into our ESLINT config to begin with.
There was a problem hiding this comment.
will take this discussion offline
There was a problem hiding this comment.
TODO: figure a way to declare these variables only in the frontend client directory. Strangely, there is already a helper.d.ts file that declares these two variables as strings.
src/client/app/helpers/helpers.d.ts
Outdated
| declare let DD_SERVICE: string | ||
| declare let DD_ENV: string |
There was a problem hiding this comment.
Related to previous comments as well. This won't be necessary if we use process.env.
There was a problem hiding this comment.
This will be taken offline as well
There was a problem hiding this comment.
updates: worth trying to get this to work by itself without the help of the eslintrc config file. maybe this declaration file needs to be directly imported or something? 🤔
Problem
datadog integration to client side
Solution
using datadog/browser-rum lib