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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Dec 18, 2023
Merged

feat: add message to warn users about metrics collection #6

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ec4bf0
Add message to warn users about metrics collection
peter-rr Dec 13, 2023
74a4876
Improve warning message
peter-rr Dec 15, 2023
dad0915
Apply changes to use the logger configured in oclif
peter-rr Dec 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
try {
const {document} = await this.parser.parse(rawDocument);
if (document !== undefined) {
metadata = MetadataFromDocument(document, metadata);

Check failure on line 31 in src/base.ts

View workflow job for this annotation

GitHub Actions / Test NodeJS PR - ubuntu-latest 

Argument of type 'AsyncAPIDocumentInterface' is not assignable to parameter of type 'AsyncAPIDocument'.
}
} catch (e: any) {
if (e instanceof Error) {
Expand Down Expand Up @@ -84,6 +84,7 @@
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.');
Copy link
Owner

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.

Copy link
Owner

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.

Copy link
Collaborator Author

@peter-rr peter-rr Dec 13, 2023 •
edited
Loading

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 🤔

Copy link
Collaborator Author

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?

Copy link
Owner

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.

Copy link
Owner

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>

Copy link
Owner

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 and this.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).

break;
}
}
Expand Down
Loading