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.

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

Unify logic for metrics collection when any command is invoked #5

Merged
merged 7 commits into from
Dec 12, 2023

Conversation

peter-rr
Copy link
Collaborator

@peter-rr peter-rr commented Nov 28, 2023

Add new methods to unify logic so metrics are collected when any CLI command is invoked. All this logic will be implemented in base.ts file and removed from the different command files.

@peter-rr
Copy link
Collaborator Author

peter-rr commented Nov 28, 2023

@smoya Metrics are sent but the format of the action invoked is detected as undefined.
Working on the fix!

image

Also I need to remove the code corresponding to this metric in all the other commands.

src/base.ts Outdated Show resolved Hide resolved
src/commands/validate.ts Outdated Show resolved Hide resolved
src/base.ts Outdated Show resolved Hide resolved
src/base.ts Outdated Show resolved Hide resolved
@peter-rr
Copy link
Collaborator Author

peter-rr commented Dec 5, 2023

@smoya
Some notes from last commit:

  • Still figuring out the way to access some variables from the different command files, as you can see here.
  • I'm considering to add any method which returns the appropriate rawDocument depending on the command executed. Or maybe this might be added inside callable function.
  • The commented code from metrics recording on the different command files will be removed. Just keeping it for testing purposes by now.

@peter-rr peter-rr changed the title Unify logic for metrics collection when any command is invoked Unify logic for metrics collection Dec 5, 2023
src/base.ts Outdated
@@ -29,6 +31,28 @@ export default abstract class extends Command {
const {document} = await this.parser.parse(rawDocument);
if (document !== undefined) {
metadata = MetadataFromDocument(document, metadata);
const commandName : string = this.id || '';
switch (commandName) {
case 'validate':
Copy link
Owner

@smoya smoya Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about the reason you moved all the logic from each command to this big switch statement here.

As I see, this is going into a bit of a deep hole. Do you think we could roll back this (last changes) and just merge the PR so we can move forward and start collecting metrics? Then we could work on this later.

The extended side of this comment for future work:

We have what we would like to have ideally, a way of tracking, from the base.ts, that the command was executed and includes some metadata, if exists.
But I would say nothing about moving all the metadata collection logic to the base.ts file. This should stay in each command as it is the responsibility of each command to collect such metadata.

Instead of doing this, have you thought about the possibility of setting all this metadata into a property declared in the base.ts file but set on each command? I mean, something like this.metadata['success'] = true; but from the command. Then, base.ts could grab that metadata.

@peter-rr peter-rr changed the title Unify logic for metrics collection Unify logic for metrics collection when any command is invoked Dec 11, 2023
@peter-rr
Copy link
Collaborator Author

@smoya Last changes already rolled back so we can merge this PR and start collecting metrics.

@peter-rr
Copy link
Collaborator Author

peter-rr commented Dec 11, 2023

@smoya What about this failing job?
Should we change something related to @asyncapi/cli dependency? 🤔
Same error at my local env when executing npm run build.

@smoya
Copy link
Owner

smoya commented Dec 12, 2023

@smoya What about this failing job? Should we change something related to @asyncapi/cli dependency? 🤔 Same error at my local env when executing npm run build.

No, it is an general issue in the CLI repository. https://asyncapi.slack.com/archives/CQVJXFNQL/p1702289410144129

Copy link
Owner

@smoya smoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀🌔

@smoya smoya merged commit 03eaf22 into smoya:feat/adoptionMetrics Dec 12, 2023
3 of 14 checks passed
@peter-rr peter-rr deleted the feat/adoptMetrics-v2 branch February 2, 2024 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants