Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

fix/frontend/graphql: log and add trace events for all non-not-found errors that occur when fetching permission syncs#63302

Merged
ggilmore merged 1 commit into
mainfrom
graphite-ggilmorefix_frontend_graphql_log_and_add_trace_events_for_all_non-not-found_errors_that_occur_when_fetching_permission_syncs
Jun 17, 2024
Merged

fix/frontend/graphql: log and add trace events for all non-not-found errors that occur when fetching permission syncs#63302
ggilmore merged 1 commit into
mainfrom
graphite-ggilmorefix_frontend_graphql_log_and_add_trace_events_for_all_non-not-found_errors_that_occur_when_fetching_permission_syncs

Conversation

@ggilmore

@ggilmore ggilmore commented Jun 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes https://linear.app/sourcegraph/issue/SRC-421/customer-reported-an-issue-with-user-permissions-stats

This PR ensures that any non not-found errors that occur when fetching permission sync jobs are logged and added as error events in the trace. Before, these errors were silently swallowed which makes things exceptionally hard to debug.

This PR only changes the monitoring around this query - it doesn't change the underlying behavior.

Note: I spent a while reading up on how to communicate partial errors in our graphql API, but it seems like there isn't a clear consensus.

According to the above, it seems like ideally we should be returning null in the graphQL for permission sync jobs that have an error. However, when I tried to do this I begain seeing nil panics all over the place. It's hard for me to know if the underlying library supports this or if we are just holding it wrong. 😥

Test plan

Unit tests

Changelog

  • Our graphqlAPI now logs and traces any non-not-found errors that occur when fetching permission sync jobs (as opposed to being silently swallowed).

@cla-bot cla-bot Bot added the cla-signed label Jun 17, 2024
@github-actions github-actions Bot added team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all labels Jun 17, 2024
This was referenced Jun 17, 2024
Merged
Merged
@ggilmore Graphite App

ggilmore commented Jun 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @ggilmore and the rest of your teammates on Graphite Graphite

@ggilmore ggilmore mentioned this pull request Jun 17, 2024
Merged
@ggilmore ggilmore force-pushed the graphite-ggilmore06-17-feature_internal_database_add_test_to_ensure_that_not_found_errors_fulfiill_errcode.notfound branch from 4d5e82c to c3c0cf5 Compare June 17, 2024 19:49
@ggilmore ggilmore force-pushed the graphite-ggilmorefix_frontend_graphql_log_and_add_trace_events_for_all_non-not-found_errors_that_occur_when_fetching_permission_syncs branch from 380a90a to 688cede Compare June 17, 2024 19:49
@ggilmore ggilmore requested a review from a team June 17, 2024 19:54
@ggilmore ggilmore marked this pull request as ready for review June 17, 2024 20:04
eseliger
eseliger approved these changes Jun 17, 2024
View reviewed changes

@eseliger eseliger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice tests

@graphite-app

graphite-app Bot commented Jun 17, 2024

Copy link
Copy Markdown

Video gif. A toddler grins wide, raising their hand and giving an exuberant thumbs up at us. Their nose krinkles under their big smile. (Added via Giphy)

@ggilmore ggilmore force-pushed the graphite-ggilmorefix_frontend_graphql_log_and_add_trace_events_for_all_non-not-found_errors_that_occur_when_fetching_permission_syncs branch from 688cede to 054ced3 Compare June 17, 2024 20:16
@ggilmore ggilmore force-pushed the graphite-ggilmore06-17-feature_internal_database_add_test_to_ensure_that_not_found_errors_fulfiill_errcode.notfound branch from c3c0cf5 to fd80f0c Compare June 17, 2024 20:19
@ggilmore ggilmore force-pushed the graphite-ggilmorefix_frontend_graphql_log_and_add_trace_events_for_all_non-not-found_errors_that_occur_when_fetching_permission_syncs branch from 054ced3 to d877433 Compare June 17, 2024 20:19
@ggilmore ggilmore force-pushed the graphite-ggilmore06-17-feature_internal_database_add_test_to_ensure_that_not_found_errors_fulfiill_errcode.notfound branch from fd80f0c to 68da82c Compare June 17, 2024 20:28
@ggilmore ggilmore force-pushed the graphite-ggilmorefix_frontend_graphql_log_and_add_trace_events_for_all_non-not-found_errors_that_occur_when_fetching_permission_syncs branch from d877433 to d0fb084 Compare June 17, 2024 20:28
@ggilmore ggilmore force-pushed the graphite-ggilmore06-17-feature_internal_database_add_test_to_ensure_that_not_found_errors_fulfiill_errcode.notfound branch from 68da82c to b7026c4 Compare June 17, 2024 20:35
@ggilmore ggilmore force-pushed the graphite-ggilmorefix_frontend_graphql_log_and_add_trace_events_for_all_non-not-found_errors_that_occur_when_fetching_permission_syncs branch from d0fb084 to 782e733 Compare June 17, 2024 20:36
@ggilmore ggilmore changed the base branch from graphite-ggilmore06-17-feature_internal_database_add_test_to_ensure_that_not_found_errors_fulfiill_errcode.notfound to graphite-base/63302 June 17, 2024 20:43
@graphite-app graphite-app Bot changed the base branch from graphite-base/63302 to main June 17, 2024 20:43
@ggilmore
fix/frontend/graphql: log and add trace events for all non-not-found …
79a43a0 
…errors that occur when fetching permission syncs
@ggilmore ggilmore force-pushed the graphite-ggilmorefix_frontend_graphql_log_and_add_trace_events_for_all_non-not-found_errors_that_occur_when_fetching_permission_syncs branch from 782e733 to 79a43a0 Compare June 17, 2024 20:43
@ggilmore ggilmore merged commit adbd4b4 into main Jun 17, 2024
@ggilmore ggilmore deleted the graphite-ggilmorefix_frontend_graphql_log_and_add_trace_events_for_all_non-not-found_errors_that_occur_when_fetching_permission_syncs branch June 17, 2024 20:49
@ggilmore Graphite App

ggilmore commented Jun 17, 2024

Copy link
Copy Markdown
Contributor Author

Merge activity

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@eseliger eseliger eseliger approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ggilmore @eseliger