-
Notifications
You must be signed in to change notification settings - Fork 135
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
Capture action plugin metrics #971
Conversation
🦋 Changeset detectedLatest commit: 27ca0dd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
just a very minor comment, every thing else looks good!
packages/browser/src/plugins/remote-loader/__tests__/action-destination.test.ts
Show resolved
Hide resolved
@@ -79,7 +79,21 @@ export class ActionDestination implements DestinationPlugin { | |||
transformedContext = await this.transform(ctx) | |||
} | |||
|
|||
await this.action[methodName]!(transformedContext) | |||
try { | |||
ctx.stats.increment('analytics_js.action_plugin.invoke', 1, [ |
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.
This will let us easily aggregate these on datadog??? Sweet
|
||
await this.action[methodName]!(transformedContext) | ||
} catch (error) { | ||
ctx.stats.increment('analytics_js.action_plugin.invoke.error', 1, [ |
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.
We are repeating these strings / args . Can we save on bundle size through helper functions? Ditto elsewhere
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.
gzip is extremely good at handling repeating strings, I've verified it - sometimes placing the strings inline yields a smaller size than storing it in a variable.
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.
Even so, I still think DRY would be an improvement here for the usual reasons (eg less code/ease of update)?
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.
It's like this all across the codebase, we should refactor them all at once so that the declaration and usage is standardized all across, and not fragment it even further.
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.
Fair
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 see the benefit of storing in a variable more that we have 1 place to update/less chance of a typo/forgetting to update the value if we change it in the future. Won't block, but that would be my general guidance when we've got a string value that acts as a key in downstream.
@@ -0,0 +1,5 @@ | |||
--- | |||
'@segment/analytics-next': minor |
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.
Should this be a patch, since it's not a user-facing feature?
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 saw it more as a "non-breaking enhancement" than a bug fix - so went with it
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.
Would vote for patch since it isn't a user-facing change to any API/API behavior.
Just like how we capture metrics relating to performance of classic integrations, this patch now enables capturing of metrics related to action destinations as well.