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

Add discussion metrics weekly slack message #673

Merged
merged 2 commits into from
Oct 25, 2023
Merged

Add discussion metrics weekly slack message #673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
58854b2
add discussion metrics weekly slack message
hubertdeng123 Oct 25, 2023
6c627d1
add overflowing discussion title test case
hubertdeng123 Oct 25, 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
1 change: 1 addition & 0 deletions .env.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ FEED_DEV_INFRA_CHANNEL_ID="13141516"
FEED_ENGINEERING_CHANNEL_ID="33333333"
DISABLE_GITHUB_METRICS="false"
FORCE_GET_USER_BY_SLACK_ID=
EPD_LEADERSHIP_CHANNEL_ID=
TEAM_OSPO_CHANNEL_ID=
SUPPORT_CHANNEL_ID=
DRY_RUN=
4 changes: 3 additions & 1 deletion src/config/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ export const DISABLE_GITHUB_METRICS =
export const DRY_RUN =
process.env.DRY_RUN === 'true' || process.env.DRY_RUN === '1';
export const PROJECT =
process.env.ENV === 'production' ? 'super-big-data' : 'sentry-dev-tooling';
process.env.ENV === 'production'
? 'super-big-data'
: process.env.DEV_GCP_PROJECT;

// The name of the GitHub Check that is created in getsentry to aggregate "required" jobs
export const REQUIRED_CHECK_NAME = 'getsentry required checks';
Expand Down
47 changes: 45 additions & 2 deletions src/utils/scores.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ jest.mock('@google-cloud/bigquery', () => ({
};
},
}));
import { getIssueEventsForTeam } from './scores';
import { getDiscussionEvents, getIssueEventsForTeam } from './scores';

describe('score tests', () => {
afterEach(() => {
jest.clearAllMocks();
});
it('should send the right sql we expect', () => {
it('should send the right sql we expect for getIssueEventsForTeam', () => {
getIssueEventsForTeam('team-ospo');
const query = `WITH labelings AS (
SELECT
Expand Down Expand Up @@ -98,4 +98,47 @@ describe('score tests', () => {
;`;
expect(mockQuery).toHaveBeenCalledWith(query);
});

it('should send the right sql we expect for getDiscussionEvents', async () => {
await getDiscussionEvents();
const discussionsQuery = `
SELECT
discussions.target_name as title,
discussions.repository as repository,
discussions.object_id as discussion_number,
COUNT(discussions.target_name) as num_comments,
FROM
\`open_source.github_events\` AS discussions
WHERE
discussions.type = 'discussion_comment'
AND timestamp_diff(
CURRENT_TIMESTAMP(),
discussions.created_at,
day
) <= 7
GROUP BY discussions.target_name, discussions.repository, discussions.object_id
ORDER BY num_comments DESC
;`;

const discussionCommentersQuery = `
SELECT
discussions.username as username,
COUNT(discussions.username) as num_comments,
FROM
\`open_source.github_events\` AS discussions
WHERE
discussions.type = 'discussion_comment'
AND timestamp_diff(
CURRENT_TIMESTAMP(),
discussions.created_at,
day
) <= 7
AND discussions.user_type != 'external'
AND discussions.user_type != 'bot'
GROUP BY discussions.username
ORDER BY num_comments DESC
;`;
expect(mockQuery).toHaveBeenCalledWith(discussionsQuery);
expect(mockQuery).toHaveBeenCalledWith(discussionCommentersQuery);
});
});
51 changes: 51 additions & 0 deletions src/utils/scores.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,54 @@ export async function getIssueEventsForTeam(team) {

return issues;
}

export async function getDiscussionEvents() {
const discussionsQuery = `
SELECT
discussions.target_name as title,
discussions.repository as repository,
discussions.object_id as discussion_number,
COUNT(discussions.target_name) as num_comments,
FROM
\`open_source.github_events\` AS discussions
WHERE
discussions.type = 'discussion_comment'
AND timestamp_diff(
CURRENT_TIMESTAMP(),
discussions.created_at,
day
) <= 7
GROUP BY discussions.target_name, discussions.repository, discussions.object_id
ORDER BY num_comments DESC
;`;

const [discussions] = await bigqueryClient.query(discussionsQuery);

const discussionCommentersQuery = `
SELECT
discussions.username as username,
COUNT(discussions.username) as num_comments,
FROM
\`open_source.github_events\` AS discussions
WHERE
discussions.type = 'discussion_comment'
AND timestamp_diff(
CURRENT_TIMESTAMP(),
discussions.created_at,
day
) <= 7
AND discussions.user_type != 'external'
AND discussions.user_type != 'bot'
GROUP BY discussions.username
ORDER BY num_comments DESC
Comment on lines +108 to +123
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope for this PR, but I wonder if there is some standard way of running a local in-memory instance of SQLite to query against, or something similar? Otherwise we end up with a ton of untested code: we validate that the exact text of our SQL code is what we expect, but not what that SQL code actually does (ie, does it get the exact rows in the table we want it to?).

I could imagine using in-memory SQLite as our backend, since this looks like bog-standard SQL with no BigQuery specific extensions in it.

Just a thought - not a blocker by any means, and definitely not a thing to do in this PR, but something to think about, since we've had a few cases of queries not returning the data set we expect them to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that'd be nice to just to get some validation that it's valid sql. Adding to the backlog of things to do for eng-pipes:
#676

Copy link
Member Author

Choose a reason for hiding this comment

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

but yes, the dev environment setup for BigQuery leaves much to be desired....

I don't think most people who have worked in eng-pipes have even hooked up their env to BigQuery

;`;

const [discussionCommenters] = await bigqueryClient.query(
discussionCommentersQuery
);

return {
discussions,
discussionCommenters,
};
}
Loading
Loading