Skip to content

Rethink how we handle the integrations with multiple metrics libraries #89

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

Closed
emschwartz opened this issue May 17, 2023 · 4 comments · Fixed by #97
Closed

Rethink how we handle the integrations with multiple metrics libraries #89

emschwartz opened this issue May 17, 2023 · 4 comments · Fixed by #97
Assignees
Labels
question Further information is requested
Milestone

Comments

@emschwartz
Copy link
Contributor

To date, we've abstracted away the differences between the underlying metrics libraries in the tracker module. We implement the TrackMetrics trait for each metrics library and then export the specific implementation as autometrics::__private::AutometricsTracker (renaming it from its library-specific struct name), depending on which feature flag(s) you have enabled. The autometrics macro generates code that uses the AutometricsTracker struct.

This works okay as long as every library has a close enough API that we don't need to expose library-specific things. However, that changes a bit with the prometheus-client support (#25 and #88), because we need to expose a handle to the Registry. We can simply expose that as a top-level export from the crate, but it seems somewhat likely that the list of things in this category will grow.

The other thing that's a bit odd with how we're currently handling things is the precedence order for the feature flags related to the metrics libraries. We currently allow multiple to be set and then use an arbitrary order for which should take precedence. It seems like this can cause somewhat unexpected behavior, because it's not obvious from the outside which library would take precedence.

Some options we have for resolving these issues:

  • Expose metrics library-specific functionality from submodules such as integrations::prometheus, integrations::prometheus_client, etc
  • Make the metrics library feature flags mutually exclusive so you can only choose one and it's clear which one you're getting
    • This would probably mean we would want to remove opentelemetry as the default and make it clear in the docs that you need to pick which underlying library it's going to use. We can have examples in the docs that use one so that would serve as an informal default
  • Alternatively, we could make it so that if you have multiple metrics libraries enabled, autometrics would track the metrics with all of them. This would be the least strict option, but it seems somewhat likely that people would end up collecting duplicate metrics

Thoughts?

@emschwartz emschwartz added the question Further information is requested label May 17, 2023
@mellowagain
Copy link
Member

I think the choice is really between the first and second, as I really can't think of any use case where one would want to collect metrics for multiple libraries.

I think the first option is good for applications which are meant to be self-hosted/used by a end-user, not the developers themselves. That would give power to the application developers to give end users the ability to switch between the metrics library used and configuring it without writing whacky code.

The second option sounds also good though. In the Rust ecosystem there are a lot of crates which have mutually-exclusively feature flags, most notably sqlx. If we document this behavior correctly, we should be fine, something along the lines of:

#[cfg(not(any(feature = "abc", feature = "xyz")))]
compile_error!("select either feature abc or xyz");

However, we must take care of maintaining a somehow similar API interface between the clients, else our docs will end up being really, really confusing.

Option one gives us the best flexibility, but option two gives us the best API interface and thus developer usage (with the risk of having confusing documentation).

Considering the prospect that Autometrics will continue to grow, I would go with the first option, as this gives us separation already early in the project (and is easier to maintain).

@emschwartz
Copy link
Contributor Author

Good points.

Maybe we can use both the first and second ideas in the following way:

  • For each metrics library we support, have a submodule inside a module such as integrations (though I don't really like that name)
  • Each one will export a struct that implements a common TrackMetrics trait, just like we have it now
  • Maybe instead of being exported from autometrics::__private, we'll make them public?
  • Alongside that struct that implements the tracking functionality, we can have each module include whatever library-specific stuff is needed/available
  • The root integrations module (or whatever we end up calling it) can export a single AutometricsTracker (name TBD) like it does now, and we'll use feature flags to determine which one it uses
  • We'll use compiler errors like you propose to make the metrics libraries' feature flags mutually exclusive (except when the docs are being built so you can build them with --all-features)

I think all of this would work. The only thing I'm doubting now is whether we want the library-specific tracker thing to be part of the crate's public API. We may have reasons to change that and we have more flexibility if we consider it part of the private API that's only meant to be used by the macro. 🤔

@gagbo
Copy link
Member

gagbo commented May 17, 2023

I'm always afraid of building packages that have mutually exclusive features because of feature unification but I suppose autometrics is only targetting end applications so there's probably going to be a single instance of it in the dependency tree.

I don't really have an opinion on the integrations part, for the Go implementation I decided to focus all the implementation-specific bits within the init function call, so that all the rest can still use the exact same API. This way I know there's a single location to look for for differences, and all the rest of the implementation can stay hidden/private for breaking changes. As long as there's a compilation error when switching metric implementation without fixing the init call, all the differences can just be exposed through this single entry point

@emschwartz
Copy link
Contributor Author

That's a good point about feature unification. I guess it would still be okay as long as any libraries that include autometrics don't pick one of the metrics libraries (and we made it so it would still compile even if you don't)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants