-
-
Notifications
You must be signed in to change notification settings - Fork 0
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 message to warn users about metrics collection #6
feat: add message to warn users about metrics collection #6
Conversation
@smoya The message I've added is just some generic message. Feel free to suggest something more specific or accurate. |
src/base.ts
Outdated
@@ -84,6 +84,7 @@ function recorderFromEnv(prefix: string): Recorder { | |||
case 'production': | |||
// NODE_ENV set to `production` in bin/run_bin, which is specified in 'bin' package.json section | |||
sink = new NewRelicSink('eu01xx73a8521047150dd9414f6aedd2FFFFNRAL'); | |||
console.log('IMPORTANT MESSAGE: We are tracking metrics anonymously from the commands you are executing just for statistical purposes.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should better use this.log
so we use the logger configured in Oclif. In fact, not sure if better log it as a warning, so then this.warn
could be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, I would improve the message by adding instructions on how users can disable metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should better use this.log so we use the logger configured in Oclif. In fact, not sure if better log it as a warning, so then this.warn could be used.
I'm trying to use both this.log
and this.warn
but the warning message is not shown then...weird 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, I would improve the message by adding instructions on how users can disable metrics.
I'm thinking about adding something like:
If you want to disable this tracking, please set the 'ASYNCAPI_METRICS' env variable to 'false' when executing the command.
For example, run the following command instead:
>> ASYNCAPI_METRICS=false asyncapi validate spec_file.yaml
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about adding something like:
Based on your message, this is what ChatGPT gave me as simplification:
To disable tracking, set the 'ASYNCAPI_METRICS' env variable to 'false' when executing the command. For instance:
ASYNCAPI_METRICS=false asyncapi validate spec_file.yaml
Looks simpler and shorter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, I feel we need to add something like the following in the very beginning.
AsyncAPI anonymously tracks command executions to improve the specification and tools, ensuring no sensitive data reaches our servers.
It aids in comprehending how AsyncAPI tools are used and adopted, facilitating ongoing improvements to our specifications and tools.
<the rest here>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to use both
this.log
andthis.warn
but the warning message is not shown then...weird 🤔
They should work. The issue is that you are calling those from a function that has no this
scope because is not part of the base command class. We either need to pass a command to that function or rather add this function as part of the base class (second is easier and could also make sense).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nooice!!
Description
We are adding a message to warn users about metrics collection. This scenario will only take place when
NODE_ENV
variable is set toproduction
in 'bin/run_bin', which is specified in 'bin' package.json section.