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

add error metrics #120

Merged
merged 3 commits into from
Nov 1, 2023
Merged

add error metrics #120

merged 3 commits into from
Nov 1, 2023

Conversation

soupi
Copy link
Contributor

@soupi soupi commented Oct 31, 2023

What

We'd like to monitor the kinds of errors we run into to measure the connector's health, so we add counters for errors according to a few categorizations.

How

We add counters for the following metrics:

/// A collection of metrics indicating errors.
#[derive(Debug, Clone)]
pub struct ErrorMetrics {
    /// the connector received an invalid request.
    invalid_request_total: IntCounter,
    /// the connector received a request using capabilities it does not support.
    unsupported_capability_total: IntCounter,
    /// the connector could not fulfill a request because it does not support
    /// certain features (which are not described as capabilities).
    unsupported_feature_total: IntCounter,
    /// the connector had an internal error.
    connector_error_total: IntCounter,
    /// the database emmited an error.
    database_error_total: IntCounter,
}

And use ndc-postgres errors to find and count said errors.

@@ -17,9 +17,20 @@ pub enum Error {
EmptyPathForStarCountAggregate,
NoFields,
TypeMismatch(serde_json::Value, database::ScalarType),
CapabilityNotSupported(UnsupportedCapabilities),
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we don't actually use this; why is it necessary?

Copy link
Contributor Author

@soupi soupi Oct 31, 2023

Choose a reason for hiding this comment

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

Future proofing and documentation. If you don't like it I can remove it :)

@soupi soupi marked this pull request as ready for review November 1, 2023 08:28
@@ -15,7 +15,7 @@ name = "ndc-postgres"
path = "bin/main.rs"

[dependencies]
ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "5b8761b7" }
ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "619655a" }
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume we're changing to use the new reverted-metrics commit of ndc-hub?

Copy link
Contributor Author

@soupi soupi Nov 1, 2023

Choose a reason for hiding this comment

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

I didn't bother updating the hash because that commit is just a revert so effectively it's the same code, so I reverted the update commit.

#[derive(Debug, Clone)]
pub struct ErrorMetrics {
/// the connector received an invalid request.
invalid_request_total: IntCounter,
Copy link
Contributor

Choose a reason for hiding this comment

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

We have another ticket for counting failed connection acquisition, so this grouping will be helpful for that.

@soupi soupi added this pull request to the merge queue Nov 1, 2023
Merged via the queue into main with commit ed67719 Nov 1, 2023
@soupi soupi deleted the gil/add-error-metrics branch November 1, 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.

3 participants