vector: Improve error type handling

Many of Vector’s methods use a Result<_, String> return type. This is adequate for textual errors, but it makes it impossible for users to pattern match on the error type as well as dropping any cause information encapsulated by some error types. We should switch to proper error types (similar to std::io::Error) probably aided by the failure crate.

References:

About this issue

  • Original URL
  • State: closed
  • Created 5 years ago
  • Reactions: 2
  • Comments: 24 (24 by maintainers)

Most upvoted comments

I’m not sure that it’s very high priority, but there is definitely still work that we could do to improve our error handling story. This initial pass at this used snafu, but there have been quite a few uses of anyhow/thiserror that have crept into the codebase over time, as well as a few areas where I think we’re still using String as our error type. There are also some newer crates like color-eyre that have some nice integrations with tracing spans that could be useful for us. Overall, I think it would definitely be of some value to review what we’re doing and choose one consistent pattern.

I would say we should take a quick inventory of components still using String errors and either add a checklist to this issue or break them out into individual issues so we can track overall progress (probably just a checklist here).

I think the preferred path would be to introduce a enum ConfigError or equivalent and turn all those returns into Result<…, Vec<ConfigError>>. At some layer, they may have to be boxed, but at the point they are emitted they can be enumerated and structured. Wrapping the error vector in a newtype as you describe would be a useful additional abstraction, but since they are all config errors, they can stay enumerated there.

The whole configuration parsing stack still uses String error returns, mostly wrapped into Vec<String>. I have started working on this but pushed it aside to work on higher priority issues (TLS mostly). It was turning out to be less of a straightforward conversion that the sinks and sources were.

A quick grep shows at least the following modules:

  • src/buffers/disk.rs
  • src/region.rs
  • src/sinks/aws_cloudwatch_logs/mod.rs
  • src/sinks/aws_cloudwatch_metrics.rs
  • src/sinks/aws_kinesis_streams.rs
  • src/sinks/aws_s3.rs
  • src/sinks/blackhole.rs
  • src/sinks/clickhouse.rs
  • src/sinks/console.rs
  • src/sinks/elasticsearch.rs
  • src/sinks/http.rs
  • src/sinks/kafka.rs
  • src/sinks/mod.rs
  • src/sinks/prometheus.rs
  • src/sinks/splunk_hec.rs
  • src/sinks/tcp.rs
  • src/sinks/util/http.rs
  • src/sinks/vector.rs
  • src/sources/file.rs
  • src/sources/statsd/mod.rs
  • src/sources/statsd/parser.rs
  • src/sources/stdin.rs
  • src/sources/syslog.rs
  • src/sources/tcp.rs
  • src/sources/udp.rs
  • src/sources/util/tcp.rs
  • src/sources/vector.rs
  • src/topology/config/mod.rs
  • src/transforms/add_fields.rs
  • src/transforms/coercer.rs
  • src/transforms/field_filter.rs
  • src/transforms/grok_parser.rs
  • src/transforms/json_parser.rs
  • src/transforms/log_to_metric.rs
  • src/transforms/lua.rs
  • src/transforms/regex_parser.rs
  • src/transforms/remove_fields.rs
  • src/transforms/sampler.rs
  • src/transforms/tokenizer.rs
  • tests/crash.rs