-
Notifications
You must be signed in to change notification settings - Fork 129
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
Enabling tracing by usage of opentelemetry #99
base: new-index
Are you sure you want to change the base?
Conversation
Isn't |
They both enhance observability but are complementary. |
I'm still not quite sure I understand the differences. Prometheus can and is being used to track function execution times, for example in these functions (via + #[instrument(skip_all, name="Mempool::utxo")]
pub fn utxo(&self, scripthash: &[u8]) -> Vec<Utxo> {
let _timer = self.latency.with_label_values(&["utxo"]).start_timer();
let entries = match self.history.get(scripthash) {
@@ -209,6 +216,7 @@ impl Mempool {
.collect()
}
+ #[instrument(skip_all, name="Mempool::stats")]
// @XXX avoid code duplication with ChainQuery::stats()?
pub fn stats(&self, scripthash: &[u8]) -> ScriptStats {
let _timer = self.latency.with_label_values(&["stats"]).start_timer();
@@ -258,12 +266,14 @@ impl Mempool {
stats
}
+ #[instrument(skip_all, name="Mempool::txids")]
// Get all txids in the mempool
pub fn txids(&self) -> Vec<&Txid> {
let _timer = self.latency.with_label_values(&["txids"]).start_timer();
self.txstore.keys().collect()
} |
It's not critical, but it could aid review if the addition of |
…-type Add support for anchor output type
db37cef
to
0441c07
Compare
How do you feel about defining a no-op |
I found out that instead of defining my own procedural macro I can just use |
Tracing is turned off by default and can be turned on by providing following params for both
|
It would be nice if it was possible to avoid the additional dependencies altogether, not just disable tracing |
I guess that needs, as you suggested, a new macro that is no-op in case of otlp-tracing feature flag is off. You're right that we still have dependency on new crates. But by default the only new crates we will depend on is just one: tracing - so that we can turn off it's macro that we used. Only if we turn on future flag we pull other dependencies in addition to tracing: so we add 1 new dependency only (or 6 if one decides to use tracing). Is it still not acceptable @shesek ? |
I had a preference to avoid additional dependencies altogether, and thought this should be as simple as #[not(cfg(feature = "otlp"))]
#[proc_macro_attribute]
pub fn instrument(_attr: TokenStream, item: TokenStream) -> TokenStream { item } However I realized that this would require making this a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a suggestion and some nits (with apologies for the newline pedantry 😅)
It's kind of unfortunate that we have to use a negative feature, which requires using --no-default-features
to disable no-otlp-tracing
when enabling otlp-tracing
. It'll become more annoying if we ever add other default features, as they'll have to be re-enabled manually. But I don't see a way around it... is there? (/cc @RCasatta)
One thing we could do to make this less of a footgun is to add some explicit useful error messages when #[cfg(not(any(feature = "otlp-tracing", feature = "no-otlp-tracing")))]
compile_error!("Must enable one of the 'otlp-tracing' or 'no-otlp-tracing' features");
#[cfg(all(feature = "otlp-tracing", feature = "no-otlp-tracing"))]
compile_error!("Cannot enable both the 'otlp-tracing' and 'no-otlp-tracing' features"); |
By looking here https://github.com/tokio-rs/tracing/blob/bdbaf8007364ed2a766cca851d63de31b7c47e72/tracing/src/level_filters.rs#L68 it seems that maximum tracing is the default so we don't need to opt-in for it? Thus, it seems to me we can avoid the positive feaure? |
The original goal for using the feature was to make (most) of the additional dependencies optional, which (I think?) we can't do with just a negative feature |
I see... I understand the issue of having too many deps, but I think it's greater in "money-handling" libs such as wallets than in electrs, so I would bite the bullet and build some deps even though they are not used instead of adding complexity. |
Yes, that was the motivation behind this, keep additional dependencies at minimum and also make #instrument[] annotations no-op by default. I couldn't find other solution than |
I generally agree with the sentiment, but its a lot of new dependencies - 6 direct ones, expanded into a total of 71 new crates in the dependency graph - for a feature that won't typically be used by other users of esplora/electrs. Adding just the |
I miscalculated the crates in my previous comment - the full set of dependencies expands into 41 new crates in the graph, while |
I've added tracing to the project by introducing mainly telemetry, opentelemetry and Tokyo crates. Having tracing allows analysis of bottleneck functions (functions that take majority of execution time).