-
-
Notifications
You must be signed in to change notification settings - Fork 157
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 POC for measuring adoption #859
Conversation
Something we might want to move on is to use Oclif hooks for lifecycle events so we can create the metric metadata on the In that way we would be always sending a metric, no matters if the command fails or not, and log the cmd result (OK|KO). |
@smoya Let me work on the metrics recording implementation for all the actions/commands as you did for |
6e667b5
to
5b25de2
Compare
c9027b5
to
e9e79f6
Compare
* feat: add POC for measuring adoption * Update convert command * Add metrics recording to some commands * Add new metric recording for action invocation * Add new metric recording for another command * Reduce metrics recording code --------- Co-authored-by: Sergio Moya <[email protected]>
4fabc9c
to
3ce1520
Compare
…#5) * Add new method to unify logic so metrics are collected when any command is invoked * Remove some unnecessary code * Change NR license key * Fix 'init()' to return a proper value for the command invoked * Unify logic for the remaining commands * Unify logic also for 'action.executed' metric * Rollback last changes
Important Not ready yet, just marking as ready for review to let CI run tests |
@peter-rr anything blocking this PR from merging ?, we would like to get some adoption insights :) |
Now I just need to update the dependency with asyncapi-adoption-metrics repo and also resolve the current conflicts. I'll let you know in case I find any blocker. |
@Amzani |
Yo folks @derberg @Souvikns @magicmatatjahu , |
Quality Gate passedIssues Measures |
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.
LGTM 👍🏼
/rtm |
@Souvikns please have a look into CI. Since this PR was merged, the release pipeline fails Next PRs like -> #1297 also are not releasable for now https://github.com/asyncapi/cli/actions/workflows/if-nodejs-release.yml |
Having some issues while installing dependencies, I tried installing the dependencies locally and it works fine, I have no idea what is happening in the GitHub action. Looks like some cache error. npm ERR! code EEXIST
npm ERR! syscall open
npm ERR! path C:\npm\cache\_cacache\tmp\f46cd6fa
npm ERR! errno -4075
npm ERR! EEXIST: file already exists, open 'C:\npm\cache\_cacache\tmp\f46cd6fa'
npm ERR! File exists: C:\npm\cache\_cacache\tmp\f46cd6fa
npm ERR! Remove the existing file and try again, or run npm
npm ERR! with --force to overwrite files recklessly.
|
@Souvikns isn't it something related to this |
Looks like cleaning the cache by force, could potentially solve this, but in |
🎉 This PR is included in version 1.8.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Important
Not ready yet, just marking as ready for review to let CI run tests
This PR is meant as a simple POC for demonstrating how we could collect metrics for measuring adoption.
This code uses the following shared library https://github.com/smoya/asyncapi-adoption-metrics. It can be shared between CLI, Studio, and any other tool that wants to keep track of those metrics.
In this example, we are just measuring the happy path of
validate
CLI command. The sink (aka backend where metrics are sent) in this POC is just logging to StdOut (console.log
).The output when validating a simple and small doc atm will output the collected metrics, I.e.:
Of course, a lot of things are missing surrounding this, like the important warning to the user saying we are collecting metrics, a way to disable, etc etc etc.
Also, we might want to encapsulate this logic as well in a single file or something we can reuse for other commands in CLI
cc @Amzani @peter-rr @fmvilas @derberg
Related issue(s)
#841