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

feat!: add method for 'action.invoked' new metric to be implemented #15

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

peter-rr
Copy link
Collaborator

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

Just adding recordActionInvoke() method and its corresponding test.

This method will be called by the different CLI commands at the time they are invoked in order to send a new metric called asyncapi_adoption.action.invoked.

Relates to:
asyncapi/cli#841

@peter-rr
Copy link
Collaborator Author

peter-rr commented Nov 17, 2023

@smoya Do you like recordActionInvoke() for the name of the method or should we call it recordActionInvocation() instead, just to follow the same syntax applied to recordActionExecution()?

@smoya
Copy link
Owner

smoya commented Nov 17, 2023

@smoya Do you like recordActionInvoke() for the name of the method or should we call it recordActionInvocation() instead, just to follow the same syntax applied to recordActionExecution()?

Not strong reason to choose one or another. However, maybe it makes sense to call it recordActionInvoked and rename the other to recordActionExecuted so they match with the final metric name. WDYT?

it('recordActionInvoke()', async function() {
const recorderMetricsSpy = [];
const recorder = new Recorder('test', new testSink(), recorderMetricsSpy);
const expectedMetric = new Metric('test.action.invoked', MetricType.Counter, 1, { action: 'convert', success: true });
Copy link
Owner

Choose a reason for hiding this comment

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

I think adding success here might sound confusing, since you don't really have the data because the action didn't finished yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, makes sense 👍

@peter-rr
Copy link
Collaborator Author

@smoya Do you like recordActionInvoke() for the name of the method or should we call it recordActionInvocation() instead, just to follow the same syntax applied to recordActionExecution()?

Not strong reason to choose one or another. However, maybe it makes sense to call it recordActionInvoked and rename the other to recordActionExecuted so they match with the final metric name. WDYT?

Agree! I think that's the appropriate naming to avoid typos and possible errors 👌

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 changed the title feat: add method for 'action.invoked' new metric to be implemented feat!: add method for 'action.invoked' new metric to be implemented Nov 20, 2023
@smoya
Copy link
Owner

smoya commented Nov 20, 2023

@peter-rr I changed the conventional commit prefix to include ! so it is released as major. The reason is the renamed function recordActionExecution to recordActionExecuted

@smoya smoya merged commit 8787cff into smoya:main Nov 20, 2023
5 checks passed
@smoya
Copy link
Owner

smoya commented Nov 20, 2023

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@smoya smoya added the released label Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants