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(monitoring): track graphql errors in metrics #6087

Merged
merged 9 commits into from
Sep 29, 2022

Conversation

szalai1
Copy link
Contributor

@szalai1 szalai1 commented Sep 29, 2022

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions
Copy link

github-actions bot commented Sep 29, 2022

Unit Test Results (build & test)

584 tests  ±0   580 ✔️ ±0   12m 53s ⏱️ -21s
143 suites ±0       4 💤 ±0 
143 files   ±0       0 ±0 

Results for commit 1eaea12. ± Comparison against base commit 8163e1c.

♻️ This comment has been updated with latest results.

@@ -126,9 +126,20 @@ void getGraphQL(HttpServletRequest request, HttpServletResponse response) {
throw new UnsupportedOperationException("GraphQL gets not supported.");
}

private void observeErrors(ExecutionResult executionResult) {
executionResult.getErrors().forEach(graphQLError -> {
String errorType = graphQLError.getErrorType().toString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

So we also have our own exception type called DataHubGraphQLError that has an "error code" field. This would be useful to extract as it can show error codes like 401, 404, 500 etc.

If the first error is not an instance of DataHubGraphQLError, then we can increment he "500" error code bucket

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm seeing metrics like this if I run it locally:
metrics_com_datahub_graphql_GraphQLController_errorType_DataFetchingException_Count
DataFetchingException is the result of DataHubGraphQLError.getErrorType(): here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that what you meant?

Copy link
Collaborator

@jjoyce0510 jjoyce0510 left a comment

Choose a reason for hiding this comment

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

I think this looks good to me. Hoping you have tested this doesn't affect anything locally

@jjoyce0510
Copy link
Collaborator

Actually - looks like compilation is failing. Please address first!

Copy link
Collaborator

@RyanHolstien RyanHolstien left a comment

Choose a reason for hiding this comment

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

Fix checkstyle and then we can get this in :)

@szalai1 szalai1 merged commit 4792ac5 into datahub-project:master Sep 29, 2022
@szalai1 szalai1 deleted the measure-graphql-errors branch September 29, 2022 19:36
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.

3 participants