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.

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

feat: add api to get mau summaries #554

Merged
merged 5 commits into from Oct 26, 2023
Merged

feat: add api to get mau summaries #554

merged 5 commits into from Oct 26, 2023

Conversation

masaaania
Copy link
Contributor

@masaaania masaaania commented Oct 17, 2023

  • This PR
    • adds an API to summarize MAU. 78f10e0
    • adds a cronjob to execute that API. 6094e9d

@masaaania masaaania marked this pull request as ready for review October 18, 2023 04:02
@masaaania masaaania marked this pull request as draft October 24, 2023 06:09
@@ -130,6 +131,15 @@ message GetMAUCountResponse {
int64 user_count = 2;
}

message GetMAUCountsRequest {
string year_month = 1;
bool isGroupingBySourceID = 2;
Copy link
Member

Choose a reason for hiding this comment

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

nit: is_grouping_by_source_id

environment_namespace as environment_id,
source_id,
count(*) as user_count,
IFNULL(SUM(event_count), 0) as request_count
Copy link
Member

Choose a reason for hiding this comment

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

How about adding the evaluation and goal request count to the query?

@masaaania masaaania force-pushed the mau_sum branch 2 times, most recently from c8e7855 to 7a86be3 Compare October 25, 2023 10:21
@@ -786,6 +786,106 @@ func TestGetMAUCount(t *testing.T) {
}
}

func TestSummarizeMAUCounts(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add the tests for Unauthenticated, PermissionDenied, and Internal while calling the checkAdminRole?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cre8ivejp
Thanks, I added it 👍

Copy link
Member

@cre8ivejp cre8ivejp left a comment

Choose a reason for hiding this comment

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

Thank you 🎉

@masaaania masaaania merged commit cf09fe2 into main Oct 26, 2023
15 checks passed
@masaaania masaaania deleted the mau_sum branch October 26, 2023 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants