Update integration metrics capturing#985
Conversation
Previous attempt at capturing action metrics used new metrics key, however that requires certain updates on the production server to work. The schedule of when those updates will go live is uncertain, so we're back to using the existing framework without minor changes.
🦋 Changeset detectedLatest commit: ea45fcb 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 |
| [ | ||
| `method:${methodName}`, | ||
| `integration_name:${this.action.name}`, | ||
| `type:${this.action.type}`, |
There was a problem hiding this comment.
Shouldn't this be something like action-destination? Otherwise it's the lifecycle hook type.
There was a problem hiding this comment.
The type could be destination, before, enrichment, etc. so for the cases where it is destination it'd be semantically and technically accurate for all purposes concerned I think.
There was a problem hiding this comment.
With your previous PR, we were only tracking the fact that it is an action destination, and not the plugin type (before, enrichment, destination, after). So I thought this change would be doing the same - since you have classic-destination, that this would be action-destination. We could also add a tag for the plugin type, though I'm not sure if that's necessary since we can easily infer it from the plugin name)
There was a problem hiding this comment.
The previous PR used a more generic terminology, i.e action_plugin which didn't necessarily mean a destination - but anything "action-like".
Since in this case we're re-using existing infrastructure, and it uses the term integration, I felt it might be important to be explicit about what exactly it is. If we were only to have 2 broad categories, classic-destination and action-destination, it'd make it very hard to construct datadog queries, because then all records queried by action-destination might also include things like Braze Web Mode - debounce, which isn't a destination at all.
By isolating things like Braze web mode -debounce into their own 'scope', any queries we run won't be tainted.
There was a problem hiding this comment.
If we were only to have 2 broad categories, classic-destination and action-destination, it'd make it very hard to construct datadog queries
I don't think I see the difference - we only had 2 categories before, now we have 5 with these changes. classic-destinations are also a destination type for what it's worth. If you're worried about action-destination including non-destination plugins, we could change valid type values to classic, action, and leave room for future things?
My main concern is that this makes it harder to extend in the future. We're reserving all the plugin types to implicitly mean we're looking at an action, which can be confusing if we ever start tracking things like source middleware failures (which would fall under before plugin type), or add something in the future to replace actions.
Edit: If we need to filter on the type of action plugin, we could add a pluginType field as well, which would map to those 4 plugin types, but still make it easy to collect metrics on actions.
There was a problem hiding this comment.
So to be sure, you're proposing that the type be either classic-destination or action-destination, and not read this.action.type?
If the answer to above is yes, then should we disable metric logging for when this.action.type !== 'destination'?
There was a problem hiding this comment.
I'm proposing that the type is classic/classic-destination or action/action-destination (I don't particularly care if they include destination in the name - it's just the type of integration), and if there's value in knowing the plugin type, we add a different tag for that (maybe pluginType).
| ctx.stats.increment('analytics_js.integration.invoke.error', 1, [ | ||
| `method:${eventType}`, | ||
| `integration_name:${this.name}`, | ||
| `type:classic-destination`, |
There was a problem hiding this comment.
DRY nit: can we share the "recordMetric" fn (maybe rename recordIntegrationMetric)?
e.g. add it to the stats class OR as a helper fn that can be imported
There was a problem hiding this comment.
I thought of that too, but I'm not 100% sure of where to put it. One side of me saw Stats class as agnostic of its usage.
For now - we can leave it as-is, since we need to merge this soon.
There was a problem hiding this comment.
can we fix it since it's like a 2m change? seems like tech debt but i'll let @chrisradek decide.
Was thinking we can just put in somewhere in core/stats for now? e.g. metrics-helpers.ts
also. stats is already a subclass from CoreStats, so I also think adding specific methods to the instance seems in the spirit of things
3 participants
Previous attempt at capturing action metrics used new metrics key, however that requires certain updates on the production server to work. The schedule of when those updates will go live is uncertain, so we're back to using the existing framework without minor changes.