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

Jump to bottom

Capture action plugin metrics #971

Merged
merged 9 commits into from
Oct 18, 2023
Merged

Capture action plugin metrics #971

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b53d145
Capture action plugin metrics
zikaari Oct 16, 2023
e6f0def
Add changeset
zikaari Oct 16, 2023
cc7e7e6
remove console.log
zikaari Oct 16, 2023
0168d03
review feedback
zikaari Oct 17, 2023
c7b7325
add test-helps dev dep
zikaari Oct 18, 2023
692fb5c
revert refactor
zikaari Oct 18, 2023
c35ce16
Revert "add test-helps dev dep"
zikaari Oct 18, 2023
e46b1ac
relax test
zikaari Oct 18, 2023
27ca0dd
fix test
zikaari Oct 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
5 changes: 5 additions & 0 deletions .changeset/metal-drinks-repeat.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@segment/analytics-next': minor
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

---

Capture action plugin metrics
111 changes: 111 additions & 0 deletions packages/browser/src/plugins/remote-loader/__tests__/action-destination.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
import unfetch from 'unfetch'
import braze from '@segment/analytics-browser-actions-braze'

import { AnalyticsBrowser } from '../../../browser'
import { createSuccess } from '../../../test-helpers/factories'
import { JSDOM } from 'jsdom'

jest.mock('unfetch')
jest.mocked(unfetch).mockImplementation(() =>
createSuccess({
integrations: {},
silesky marked this conversation as resolved.
Show resolved Hide resolved
remotePlugins: [
{
name: 'Braze Web Mode (Actions)',
creationName: 'Braze Web Mode (Actions)',
libraryName: 'brazeDestination',
url: 'https://cdn.segment.com/next-integrations/actions/braze/a6f95f5869852b848386.js',
settings: {
api_key: 'test-api-key',
versionSettings: {
componentTypes: [],
},
subscriptions: [
{
id: '3thVuvYKBcEGKEZA185Tbs',
name: 'Track Calls',
enabled: true,
partnerAction: 'trackEvent',
subscribe: 'type = "track" and event != "Order Completed"',
mapping: {
eventName: {
'@path': '$.event',
},
eventProperties: {
'@path': '$.properties',
},
},
},
],
},
},
],
})
)

beforeEach(async () => {
const html = `
<!DOCTYPE html>
<head>
<script>'hi'</script>
</head>
<body>
</body>
</html>
`.trim()

const jsd = new JSDOM(html, {
runScripts: 'dangerously',
resources: 'usable',
url: 'https://localhost',
})

const windowSpy = jest.spyOn(global, 'window', 'get')
windowSpy.mockImplementation(
() => jsd.window as unknown as Window & typeof globalThis
)

const documentSpy = jest.spyOn(global, 'document', 'get')
documentSpy.mockImplementation(
() => jsd.window.document as unknown as Document
)
})

describe('ActionDestination', () => {
it('captures essential metrics when invoking methods on an action plugin', async () => {
const ajs = AnalyticsBrowser.load({
writeKey: 'abc',
plugins: [braze as any],
})

await ajs.ready()

expect(ajs.ctx?.stats.metrics).toHaveLength(1)

expect(ajs.ctx?.stats.metrics[0]).toMatchObject(
expect.objectContaining({
metric: 'analytics_js.action_plugin.invoke',
tags: [
'method:load',
'action_plugin_name:Braze Web Mode (Actions) trackEvent',
],
})
)

const trackCtx = await ajs.track('test')

const actionInvokeMetric = trackCtx.stats.metrics.find(
(m) => m.metric === 'analytics_js.action_plugin.invoke'
)

expect(actionInvokeMetric).toMatchObject(
expect.objectContaining({
metric: 'analytics_js.action_plugin.invoke',
tags: [
'method:track',
'action_plugin_name:Braze Web Mode (Actions) trackEvent',
],
})
)
})
})
36 changes: 33 additions & 3 deletions packages/browser/src/plugins/remote-loader/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, [
Copy link
Contributor

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

`method:${methodName}`,
`action_plugin_name:${this.action.name}`,
])

await this.action[methodName]!(transformedContext)
} catch (error) {
ctx.stats.increment('analytics_js.action_plugin.invoke.error', 1, [
Copy link
Contributor

@silesky silesky Oct 17, 2023 •
edited
Loading

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

Copy link
Contributor Author

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.

Copy link
Contributor

@silesky silesky Oct 17, 2023 •
edited
Loading

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)?

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair

Copy link
Contributor

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.

`method:${methodName}`,
`action_plugin_name:${this.action.name}`,
])

throw error
}

return ctx
}
Expand All @@ -101,8 +115,24 @@ export class ActionDestination implements DestinationPlugin {
return this.action.ready ? this.action.ready() : Promise.resolve()
}

load(ctx: Context, analytics: Analytics): Promise<unknown> {
return this.action.load(ctx, analytics)
async load(ctx: Context, analytics: Analytics): Promise<unknown> {
try {
ctx.stats.increment('analytics_js.action_plugin.invoke', 1, [
`method:load`,
`action_plugin_name:${this.action.name}`,
])

return await this.action.load(ctx, analytics)
} catch (error) {
ctx.stats.increment('analytics_js.action_plugin.invoke.error', 1, [
`method:load`,
`action_plugin_name:${this.action.name}`,
])

console.log(error)
oscb marked this conversation as resolved.
Show resolved Hide resolved

throw error
}
}

unload(ctx: Context, analytics: Analytics): Promise<unknown> | unknown {
Expand Down