add ENV to log snowplow events #37915
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 6b4631a...aa0a71d.
|
|
| @@ -41,6 +41,22 @@ export const trackStructEvent = (category, action, label, value) => { | |||
| }; | |||
|
|
|||
| export const trackSchemaEvent = (schema, version, data) => { | |||
| // eslint-disable-next-line no-undef | |||
| if (process?.env?.NODE_ENV === "development") { | |||
There was a problem hiding this comment.
Can we not do this by default, but require e.g. an env variable to be set up for it?
The console is already a pain to work with due to React prop-types errors.
There was a problem hiding this comment.
Good point, wrapped it in a env var
npretto
changed the title
deniskaber
reviewed
Jan 22, 2024
| @@ -41,6 +41,22 @@ export const trackStructEvent = (category, action, label, value) => { | |||
| }; | |||
|
|
|||
| export const trackSchemaEvent = (schema, version, data) => { | |||
| // eslint-disable-next-line no-undef | |||
| if (process.env.MB_LOG_ANALYTICS === "true") { | |||
| try { | |||
There was a problem hiding this comment.
Why do we need a try-catch here?
What error do you think could happen during console.log call?
There was a problem hiding this comment.
Good catch, I added it when it wasn't under a env flag and I wasn't sure what we passed as data -> if my destructuring was safe
deniskaber
reviewed
Jan 22, 2024
| @@ -238,6 +238,7 @@ const config = (module.exports = { | |||
| new NodePolyfillPlugin(), // for crypto, among others | |||
| new webpack.EnvironmentPlugin({ | |||
| WEBPACK_BUNDLE: "development", | |||
| MB_LOG_ANALYTICS: "false", | |||
There was a problem hiding this comment.
I think we should also add a script to run the project with this setting on
There was a problem hiding this comment.
I don't think that's very scalable, we can't have a command for any combination of envs.
For example almost everyday I use both yarn dev and yarn build:hot, others may use yarn dev-ee (which pretty much just sets MB_VERSION) etc
kamilmielnik
approved these changes
Jan 24, 2024
| @@ -41,6 +41,18 @@ export const trackStructEvent = (category, action, label, value) => { | |||
| }; | |||
|
|
|||
| export const trackSchemaEvent = (schema, version, data) => { | |||
| // eslint-disable-next-line no-undef | |||
| if (process.env.MB_LOG_ANALYTICS === "true") { | |||
There was a problem hiding this comment.
We should probably add this variable to frontend/src/metabase/env.js.
deniskaber
approved these changes
Jan 24, 2024
|
@npretto Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
sloansparger
pushed a commit
that referenced
this pull request
Feb 5, 2024
* console log snowplow events * wraps it in a env var to not spam the console * remove try catch now that's under a flag * moved reading the env to env.js
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This both makes testing things locally easier and also makes us more aware of the testing we do (and not do).
This would also help help PMs and PDs make sure we're tracking things as they expect.
Ideally we could do something similar on the BE
Demo