-
Notifications
You must be signed in to change notification settings - Fork 529
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
Edit GA4 Auto Metric Query & Add Audit Logging to Auto Metrics #1682
Conversation
…reating auto metrics.
Your preview environment pr-1682-bttf has been deployed. Preview environment endpoints are available at: |
…to the job to add to the audit log.
… still kick off the job to create auto metrics. This will be removed soon.
return; | ||
} | ||
|
||
const userObj: Omit< |
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've discussed removing the ability for a user to add auto metrics from within the NewDataSourceForm
- once we do that, we can remove this logic.
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 had a bunch of minor points, with renaming the event to be metric.autocreate
being the only substantive one. I'm approving anyway so I'm not blocking you.
@@ -1508,7 +1508,7 @@ export default abstract class SqlIntegration | |||
)}' AND'${formatDate(end, "yyyyMMdd")}'`, | |||
getAdditionalEvents: () => [], | |||
getMetricWhereClause: (eventName: string) => | |||
`WHERE event_name = '${eventName}'`, | |||
`WHERE _TABLE_SUFFIX BETWEEN '{{date startDateISO "yyyyMMdd"}}' AND '{{date endDateISO "yyyyMMdd"}}' AND event_name = '${eventName}'`, |
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 have updated this in: #1673 to get the intraday tables as well. So if I land that first then you will have a merge conflict. Or you can copy the code from that change.
@@ -1508,7 +1508,7 @@ export default abstract class SqlIntegration | |||
)}' AND'${formatDate(end, "yyyyMMdd")}'`, | |||
getAdditionalEvents: () => [], | |||
getMetricWhereClause: (eventName: string) => | |||
`WHERE event_name = '${eventName}'`, | |||
`WHERE _TABLE_SUFFIX BETWEEN '{{date startDateISO "yyyyMMdd"}}' AND '{{date endDateISO "yyyyMMdd"}}' AND event_name = '${eventName}'`, | |||
}; | |||
} | |||
case "rudderstack": |
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 is not relevant to this PR, but I'm curious as to why we use the switch statement instead of functions on the individual implementations, ie. in GoogleAnalytics.ts
@@ -23,6 +27,10 @@ type CreateAutoGeneratedMetricsJob = Job<{ | |||
| "dateCreated" | |||
| "dateUpdated" | |||
>[]; | |||
user: Omit< |
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.
Looks like you missed this one still.
This PR makes two changes to the auto generate metrics job.
It updates the default metric query for GA4 data sources to include an additional where clause using the
_TABLE_SUFFIX
column.It adds audit logging for when an auto metric is created. This looks to be a net-new pattern. Currently, it looks as those most of our audit logging happens directly within controllers. However, we don't create the auto metrics in the controller, but instead kick off an async job. As a result, we can't call
req.audit
in the controller, as we don't have necessary info, likemetric.id
as that has not yet been generated.As such, we're now having to get the user info and pass that into the job as well. Open to suggestions if there is a way we can abstract the logic so it's less verbose.
Testing
audits
collection in Mongo.