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

chore(metrics): Align error_type enum values #4848

Closed
wants to merge 2 commits into from

Conversation

lucperkins
Copy link
Contributor

@lucperkins lucperkins commented Nov 2, 2020

When working on Cue configuration for internal metrics in PR #4820, I noticed some discrepancies in the possible values for error_type (a tag for the processing_errors_total metric). This PR provides a convenient enum mechanism for keeping these values aligned.

@lucperkins lucperkins force-pushed the error_type-enum-value-fix branch 2 times, most recently from 14082f1 to c563424 Compare November 2, 2020 20:54
@lucperkins lucperkins marked this pull request as draft November 2, 2020 21:02
@lucperkins lucperkins marked this pull request as ready for review November 2, 2020 21:21
@binarylogic
Copy link
Contributor

This seems really close to #978. I'm wondering if we'd want to make these classifications part of the error event directly?

@lucperkins lucperkins marked this pull request as draft November 2, 2020 21:30
@lucperkins
Copy link
Contributor Author

lucperkins commented Nov 2, 2020

@binarylogic That's certainly a possibility as part of a more comprehensive refactor of how we do this. I would say, though, that this PR wouldn't fundamentally impede such an effort; in fact, such an undertaking could use the same ErrorTypes enum in their implementation. So I think this work improves the current situation at little cost to future changes, but I'm happy to defer to others if this isn't much of a win. In that case, I could repurpose this PR as a simple standardization of the existing error_type values as raw strs.

@lucperkins lucperkins marked this pull request as ready for review November 2, 2020 22:24
@binarylogic
Copy link
Contributor

Sounds good.

@lucperkins
Copy link
Contributor Author

@bruceg @JeanMertz I'm happy to defer to both of you on this. To summarize, the two questions on the table:

  1. Whether it's preferred to use enums for things like this (personally, I would argue in favor of that, as it provides a centralized "store" for possible values, which can prevent discrepancies). If there's consensus that the enum-driven approach is better in general, I'd be happy to go through and change this in other areas (the op_kind field in the Kubernetes metrics is another example).
  2. Whether the error_type should be included in the respective error structs.

But I have no dog in this fight, so I'm also happy to restrict this PR to just renaming the various error_type fields to remove inconsistencies.

@bruceg
Copy link
Member

bruceg commented Nov 3, 2020

1. Whether it's preferred to use enums for things like this (personally, I would argue in favor of that, as it provides a centralized "store" for possible values, which can prevent discrepancies).

I agree here. Anywhere we want to restrict the cardinality of something, we should be trying to use an enum. It would be nice to take it a step further yet, by only accepting those enums. This could be introducing a step where the enum is either enforced by type (ie a function for emitting the error counter) or a macro that accepts an identifier instead of an expression (so strings aren't usable).

2. Whether the `error_type` should be included in the respective error structs.

This doesn't seem to make as much sense. Each error struct will result in a single ErrorType being emitted, so including that value in the struct seems redundant.

Copy link
Contributor

@JeanMertz JeanMertz left a comment

Choose a reason for hiding this comment

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

Two minor nits, but nothing blocking, and I agree with @bruceg's comments, but would defer enum enforcement to a follow-up PR, since this seems (almost) good to go as-is.

src/internal_events/mod.rs Outdated Show resolved Hide resolved
src/internal_events/mod.rs Show resolved Hide resolved
Comment on lines 213 to 245
// Possible values for the "error_type" tag
pub enum ErrorTypes {
FieldMissing,
InvalidMetric,
MappingFailed,
MatchFailed,
ParseFailed,
RenderError,
SerializationFailed,
TargetFieldExists,
TemplateError,
TypeConversionFailed,
ValueInvalid,
}

impl ErrorTypes {
fn to_str(&self) -> &str {
match self {
ErrorTypes::FieldMissing => "field_missing",
ErrorTypes::InvalidMetric => "invalid_metric",
ErrorTypes::MappingFailed => "mapping_failed",
ErrorTypes::MatchFailed => "match_failed",
ErrorTypes::ParseFailed => "parse_failed",
ErrorTypes::RenderError => "render_error",
ErrorTypes::SerializationFailed => "serialization_failed",
ErrorTypes::TargetFieldExists => "target_field_exists",
ErrorTypes::TemplateError => "template_error",
ErrorTypes::TypeConversionFailed => "type_conversion_failed",
ErrorTypes::ValueInvalid => "value_invalid",
}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this type of structure has any real benefits over a set of constants, at least without further work on emitting these errors. It doesn't enforce (in the compiler) that we only use these values, and requires extra code at each invocation (which admittedly will be completely inlined in release builds). It does add a namespace (ErrorTypes) but that can also be provided with a module.

Signed-off-by: Luc Perkins <luc@timber.io>
Signed-off-by: Luc Perkins <luc@timber.io>
@lucperkins
Copy link
Contributor Author

lucperkins commented Nov 4, 2020

@bruceg @JeanMertz Oops, I made the changes requested by Jean without reading Bruce's comment. I have no strong feelings re: enum vs. const. Happy to defer on that if either of you feels particularly strongly. If the enum approach is okay, I'll fix the current errors; otherwise, I'll revamp.

@lucperkins
Copy link
Contributor Author

There doesn't seem to be consensus around the right way to do this, and I suspect there will at some point be a broader effort to standardize, so I'm going to close this. Happy to revisit and help out at any point.

@lucperkins lucperkins closed this Nov 25, 2020
@binarylogic binarylogic deleted the error_type-enum-value-fix branch August 3, 2021 18:02
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.

4 participants