Allow specifying result labels for enum variants

[gagbo]

Context

The target use case is when a service uses a single enumeration with all
the possible errors, but some "errors" should not be counted as failures
regarding metric collection (for example, all the 4xx errors in a HTTP
handler usually mean that the server succeeded in rejecting a bad request).

TODOs

This draft PR only has a raw implementation for now, which should allow discussing
implementation details, and the naming of the various attributes/arguments, but
it is still missing a few things.

• Add documentation on the derive macro
• Add tests for the macro (end to end with
  trybuild, probably)

[emschwartz]

It seems a little surprising to me that this would only work on enums returned as the error part of a Result. I think I would expect to derive this trait for an enum, then have that enum appear in the Ok or Error variant, or as the return type without a Result, and have it correctly override the default "ok" or "error" labels.

[gagbo]

this would only work on enums returned as the error part of a Result

I don't think there's a way to make a trait that work on "all enums" in general to be able to derive this behaviour outside of Result. The other thing is that since we know we're working with a Result, the current implementation can use the assumption to default the Get(Error|Result)Label implementation to returning err.

So to summarize:

• we cannot target "all enums" with a trait implementation (since there's not generic wrapper to use as Result<T, E>)
• therefore if we want to dodge compilation errors we need to have a default implementation of Get(Error|Result)Label for all types
• the default implementation for all types is hard to make work because then we can't infer anything for a function that just returns a basic type

In fn foo() -> u32, should the u32 implementation of Get(Result|Error)Label be ok or error ? I don't think there's a solution that works 100% of the time there.

[emschwartz]

I think it is possible to make it work using the autoref specialization trick.

We'd need to define a dummy trait that returns an empty string (or make both traits return Option<&'static str>). Then, we'd implement that for &T and all basic types.

The one subtle gotcha that I realized with @sagacity was that this only works when Rust decides which version to use in macro-generated code, not in the implementation of a trait.

[gagbo]

I don't think it's really possible, at least I can't make it work with the current version because to work transparently with Results that have enum or not I need to do:

impl<T, E> GetLabelsFromResult for Result<T, E> {
    fn __autometrics_get_labels(&self) -> Option<ResultAndReturnTypeLabels> {
        match self {
            Ok(ok) => Some((
                ok.__autometrics_get_result_label().unwrap_or(OK_KEY),
                ok.__autometrics_static_str(),
            )),
            Err(err) => Some((
                err.__autometrics_get_result_label().unwrap_or(ERROR_KEY),
                err.__autometrics_static_str(),
            )),
        }
    }
}

/// Implementation detail to get enum variants to specify their own
/// "result" label
pub trait GetResultLabel {
    /// Return the value to use for the [result](RESULT_KEY) value in the reported metrics
    fn __autometrics_get_result_label(&self) -> Option<&'static str> {
        None
    }
}
impl_trait_for_types!(GetResultLabel);

And basically, the "override" of __autometrics_get_result_label works fine only for the enum itself. So these test cases pass

#[derive(ResultLabels, Clone)]
enum MyEnum {
    #[label(result = "error")]
    Empty,
    #[label(result = "ok")]
    ClientError { inner: Inner },
    AmbiguousValue(u64),
}

fn main() {
    let is_ok = MyEnum::ClientError { inner: Inner {} };
    assert_eq!(is_ok.__autometrics_get_result_label().unwrap(), "ok");
    assert_eq!(is_ok.__autometrics_get_labels().unwrap().0, "ok");

    let err = MyEnum::Empty;
    assert_eq!(err.__autometrics_get_result_label().unwrap(), "error");
    assert_eq!(err.__autometrics_get_labels().unwrap().0, "error");

    let no_idea = MyEnum::AmbiguousValue(42);
    assert_eq!(no_idea.__autometrics_get_result_label(), None);
    assert_eq!(no_idea.__autometrics_get_labels(), None);
}

But when calling the method through a Result, I think the compiler already bakes in the actual trait/code that is going to be used, so there's no way to use any type where val.__autometrics_get_result_label() returns Some('label'). This translates to these test cases failing:

    let err_in_ok: Result<MyEnum, ()> = Ok(err.clone());
    // Fails
    assert_eq!(
        err_in_ok.__autometrics_get_labels().unwrap().0,
        "error",
        "When wrapped as the Ok variant of a result, a manually marked 'error' variant translates to 'error'."
    );
    
    let ok_in_err: Result<(), MyEnum> = Err(is_ok);
    // Fails too
    assert_eq!(
        ok_in_err.__autometrics_get_labels().unwrap().0,
        "ok",
        "When wrapped as the Err variant of a result, a manually marked 'ok' variant translates to 'ok'."
    );

[emschwartz]

@gagbo yeah. So I think instead of implementing the trait for Result, we'd need to use another dummy trait to first find out if the type is a Result. Then we'd use a match statement in the macro-generated code, rather than in the trait implementation on Result to have it use the right branch. This will look bad from the generated code side but I believe it'll all be optimized away by the compiler.

use autometrics::__private::{IsResult, IsNotResult, ResultLabel, EmptyResultLabel};

if result.__is_result() {
  match result {
    Ok(val) => val.get_label(),
    Err(val) => val.get_label(),
} else {
  result.get_label()
}

[emschwartz]

Might be worth using the spez crate or looking at how it's implemented

[gagbo]

I'll probably end up just using the crate, I don't think it'll add a lot of overhead and it would make separation clearer

[gagbo]

Yeah I've been fiddling a bit and it seems the function redirection inside a generic function (the impl of GetLabelForResult for Result) cannot work with the autoref-based specialization trick. To prevent combinatorial explosion of dummy traits for special cases combinations, I think I'll try to reexport spez from autometrics and directly generate code (in the main counter labels TokenStream) that uses spez so that compiler does the resolution at the last possible time from the original code.

This seems to have the advantage that it would become the single place to modify to add special cases for label handling

[emschwartz]

I think all of this should be in the macros crate, no? Why not just move this into the implementation of the autometrics proc macro?

[gagbo]

Having the macro separate allows to properly test for it (and the ResultLabels attribute macro) in the trybuild tests. Inlining everything would make the main instrument_function code harder to read and make it impossible to test the label logic in isolation

[gagbo]

The reason it's not in the macros crate is mostly that it's just expanding code and that I wanted the macro to access values from autometrics (the constants, the labels, the traits): since the macro is not a proc macro returning an arbitrary TokenStream, we need to have proper access to the crate (which we don't have inside autometrics-macros)

[emschwartz]

I think it would be better if we could call spez directly in the proc macro code and insert the token stream directly into the code the macro generates, instead of making it a dependency of the main autometrics crate and having the macro generate code that calls spez (unless there's a reason that cannot work)

[gagbo]

Calling the function spez::spez to get a proc_macro::TokenStream would probably work but it doesn't really bring advantages: calling spez::spez means that there would still be a dependency to spez (even if the macro is moved to autometrics-macros, it would be AM needs AM-macros needs spez), and preparing an input TokenStream from code is one additional step that would be unnecessary added complexity in the macro.

Generating the match Result… TokenStream in the instrument_function directly has the drawback of having bad testability and makes instrument_function readability worse