Adding class/struct names to the labels

[keturiosakys]

Currently, when registering metrics we don't include class names in labels any of the language implementations. Because methods maybe even more than simple functions can have overlapping names this can result in duplicated data.

Using a TypeScript (albeit a bit contrived) example to illustrate:

class Foo {
  @autometrics
  sayHi() {
    return "hi, from class Foo"
  }
}

and

const sayHi = autometrics(
  function sayHi() {
    return "hi from function"
  });

will register the same metric name + label if they're co-located in the same file. We should consider adding class names to the labels as well to help distinguish which classes and methods get called in the timeseries data.

The way I see it there are two options:

1. Add classes as another label. This would make sense semantically to distinguish classes as a separate category that a user can later filter/group by. However, this would increase metrics cardinality, which given our approach we should be careful about extending. Moreover, most of the other existing stuff (like dashboards, query helpers in the tooltips) don't currently account for classes.

2. Prepend classes to the function name. If we consider classes not important enough to warrant increasing the number of metric+label combinations, we can simply add them to the function name label, e.g.: the example above would register function.calls.count{function="Foo.sayHi"}. This would avoid likely duplication issues and would signal to the user reading the label that the data comes from a class method.

Personally, I'm leaning to the 2nd one but would be keen to hear what others think and if we have other options here.

[emschwartz]

Good point! Thanks for raising this issue.

One correction: both of these approaches would have the same label cardinality. Adding another label only increases the cardinality if there are different combinations of labels present. But that's exactly what you're trying to achieve in either case: ensure that these different functions are stored as separate time series.

I'm on the fence about this.

Adding a separate class label would make it easy to look at all the metrics for a given class in a language-agnostic way (i.e. without having any regexes to handle differences between the separators the languages use). But I'm not sure how useful that actually is.

If we had a separate label, we would need to ensure that queries always sum by (class, function, module). So it would become part of the unique identifier of every function, just like module is now.

If we merge it in, you could kind of make the case that the function label should include the full module path too... The main reason we kept it out was for legibility... 🤔

[keturiosakys]

ah thanks for the correction! In that case: maybe it would make sense to add class as a separate label then. Separate class labels can help organize the data: in something like NestJS classes are the key design pattern (e.g.: each controller/module/service is a class).

In that case I would suggest moving forward and adding class label where appropriate.

[hatchan]

I think it makes most sense to not have any specialized fields which might not mean anything in other languages (ie, what would be contained in class for the Rust version? The struct name, or would it simply be empty?).

I wonder if we can encode this information in module, which would be more a "namespace". So something like using a different separator in the top example: whatevermodule#Foo. The example underneath would then just contain whatevermodule.

[emschwartz]

I'm on the fence about adding a separate class label.

However, if we include the class/struct name in either one of the function or module labels, I'd prefer to do it without adding special syntax that's not already in the language. For example, in Rust I think it makes sense for methods to be Struct::method or in Typescript Class.method. When you're documenting methods in those languages, you'd already use that syntax to refer to the methods, so it feels like we aren't inventing something new.

[keturiosakys]

The main tradeoff for that I'm seeing is that if we'd then want to say update our universal dashboards, we'd have to write somewhat complicated regex soup to account for all the different syntax quirks.

[emschwartz]

Yeah, if we want to have logic for grouping metrics based on the class in the dashboards or other UI, that would be a strong case for making it a separate label. If we include it in one of the other labels, I would do that under the assumption that we do not need/want to use it in any external tool. Label values should probably be treated as unique in most cases.

[keturiosakys]

What are the possible substitute keys if we go with separating the class into a separate label?

I have a feeling if it's just class or struct in most modern languages (and the ones we currently support), then maybe the regex necessary to account for both cases is a bit easier to maintain than parsing it out from function label values themselves. Or if it's not relevant, that label can be ignored entirely.

[emschwartz]

Oh, just to clarify, I would only have a single label called class. Rust and any other language that doesn't have classes but has something kind of similar would just put whatever they have in there.

[gagbo]

I've always thought that the class/struct would just be part of the module, as in the end the name of a class acts as an additional scope for the name of the function.

I didn't even think of asking what other points of view were, I thought Rust was always doing that, so I actually had that implemented in the helper that lists all the the functions that have autometrics annotations:

#[test]
fn detect_trait_impl_block() {
    let source = r#"
        struct Foo{};

        #[autometrics]
        impl A for Foo {
            fn m_a() {}
        }
        "#;

    let list = list_function_names(String::new(), source, true).unwrap();

    assert_eq!(list.len(), 1);
    assert_eq!(
        list[0],
        ExpectedAmLabel {
            module: "Foo".into(),
            function: "m_a".into()
        }
    );
}

[emschwartz]

Rust is currently using the module_path macro, which doesn't include the name of the struct (playground).

[gagbo]

Oh that's too bad, I'll probably add a flag to hide structs then

[emschwartz]

In some ways in Rust, I'd expect the struct name to be part of the function rather than the module. Technically struct.foo() is just syntactic sugar for Struct::foo(&struct) so it might make sense for the function to be Struct::foo

[gagbo]

That's fair, I just thought that it was weird to have the module path separator in the function name field

[gagbo]

I'll make that change in am_list at least (having the struct name appear in the function name instead of the module name for Rust listings), and I think that's something we want to do in Autometrics.

If we have a pattern where I have multiple structs in the same module, where each struct has a new constructor, or implement the same trait, then we'd merge the stats of multiple functions under the same labels; an example would be the multiple encode_f64 functions in the prometheus crate (in prometheus::encoding::text)

[emschwartz]

Maybe one way of thinking about this is focusing on the question of whether you'd ever want to look at the metrics for a specific class as a whole.

If so, it would make sense to have a separate label.

If not and we're just using this to ensure we don't accidentally merge metrics from different functions, then it would make sense to include in the function name.

[gagbo]

If the issue is "just" to look at metrics for a class, we could also provide a UX around using regexes for the function_name label, since it's going to be pretty static

[emschwartz]

Indeed, we could.

I think if it's a use case we really want to support, it'd be worth adding the extra label. But we could consider not adding it now, experimenting using the regexes, and add it in later if it seems like it would add value.

[eliaperantoni]

What if each SDK was free to use a differently-structured Key-Value dictionary to describe the precise location of a function?

My reasoning is that, even if a class label was added, there are a lot of other scenarios that might give similar issues. For instance (also considering languages that Autometrics doesn't support yet, but might in the future):

C++

namespace A {
class MyClass {
    void my_func() {}
}
}

namespace B {
class MyClass {
    void my_func() {}
}
}

Python

File a.py

class MyClass:
    def my_func():
        pass

File b.py

# Can have classes with the same name in different scripts/modules
class MyClass:
    def my_func():
        pass

Moreover, it is my humble opinion that forcing a unified locating schema on completely different languages would result in a suboptimal solution. After all, classes don't exist in Go and Rust. Java also has inner classes to consider. And what about anonymous functions declared inside of another function's body?

Letting each SDK decide for its own language would remove these limitations.

C++ could save the location as an object like:

{
    source_file_path,
    namespace?, // Only if member of a namespace
    class_name?, // Only if function is a method
    function_name
}

while Java would do:

{
    source_file_path,
    package_full_name,
    parent_class_name?, // Only if an inner class
    class_name?, // Only if function is a method
    function_name
}

and Python:

{
    file_name,
    module_full_name,
    class_name?, // Only if function is a method
    function_name
}

I'm not sure that this is doable and practice but it's definitely a big technical challenge. What do you think?

[emschwartz]

Thanks @eliaperantoni for your input on this!

The main argument for having a unified label schema (even if it has optional labels only used by some implementations) is that it enables us to build visualization and debugging tools that work across languages. Any single implementation could add whatever labels it wants, but we can't use them in things like the Grafana dashboards unless we know what the labels mean.

We could potentially standardize a larger set of labels used to uniquely identify functions and then allow the different implementations to use the ones that make sense. I think the main downside would just be figuring out which labels are included and what their hierarchy is in a way that's true across languages.

After all, classes don't exist in Go and Rust. Java also has inner classes to consider. And what about anonymous functions declared inside of another function's body?

Definitely fair points about those cases. For Rust, I'd be comfortable using the name class to refer to structs, because that's the closest thing to the semantic meaning of the word and there isn't something else in the language that it could be confused with. Inner classes and anonymous functions are indeed tougher. I wonder whether those are things that you'd actually want to instrument with metrics in practice, or if it's an edge case we could leave out of scope for now.

[emschwartz]

Oh one other thing to consider: for any label that is necessary to uniquely identify the function in the codebase, we'd also need to add a caller_ variant as well. That's not strictly a reason to minimize the number of labels we're using to identify functions, but it gives us a bit of an incentive to err on the lower side.

[eliaperantoni]

Thank you very much for the detailed insight @emschwartz! Now that I get the bigger picture, I agree 100% with what you've said.

Among the remaining options, I personally quite like the one suggested by @gagbo to regex on the function_name label.

[emschwartz]

Of course -- thoughtful contributions always deserve detailed responses!

[emschwartz]

One other potential argument against adding a class label: this label is probably most useful in object-oriented languages. However, I think a lot of OO languages strongly encourage you to have one class per file. In that case, the function + module might already represent the same level of granularity as function + class + module would.

Folks that program in OO languages, please correct me if I'm wrong about this. cc @P2P-Nathan @jamsiedaly