-
Notifications
You must be signed in to change notification settings - Fork 76
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
Usage metrics for 2024.3 release #1345
Conversation
c7180e7
to
c4c8501
Compare
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 haven't had time to take a close look at this and probably won't have time to today. In particular, I haven't looked at tests at all. That said, things are looking good so far. 👍 I think we should go ahead and merge, then I'll review further later. I'll leave a comment on the issue or file a new issue if I notice anything.
THEN 1 ELSE 0 END) create_api_recent, | ||
e."datasetId" | ||
FROM audits a | ||
JOIN entities e on CAST((a.details ->> 'entityId'::TEXT) AS integer) = e.id |
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.
Is ::TEXT
needed?
@@ -450,6 +477,33 @@ FROM datasets ds | |||
WHERE a."action" = 'entity.update.version' | |||
GROUP BY e."datasetId" | |||
) as updates ON ds.id = updates."datasetId" | |||
LEFT JOIN ( | |||
SELECT | |||
COUNT (1) total, |
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 feel like we usually don't leave a space between the function name and the parentheses.
I've taken a closer look at everything except for tests, and things are looking good. 👍 @ktuite, how about we review tests interactively at some point? |
|
||
const datasets = await container.Analytics.getDatasets(); | ||
|
||
datasets[0].num_entity_create_sub_total.should.be.equal(0); |
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.
Run the worker and assert that this is not zero
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 looked at tests with @ktuite, and those are looking good. 👍
Closes getodk/central#722
Based on this PR where I change Promise.all to runSequentially for the bulk analytics queries. #1344
What has been done to verify that this works as intended?
Why is this the best possible solution? Were any other approaches considered?
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.
Before submitting this PR, please make sure you have:
make test
and confirmed all checks still pass OR confirm CircleCI build passes