-
Notifications
You must be signed in to change notification settings - Fork 182
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
*: add feature "push" #86
Conversation
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.
Couple of minor comments, but otherwise LGTM
@@ -66,8 +69,6 @@ pub use self::histogram::{Histogram, HistogramVec, HistogramOpts, HistogramTimer | |||
// Functions | |||
pub use self::registry::{gather, register, unregister}; | |||
pub use self::histogram::{linear_buckets, exponential_buckets}; | |||
pub use self::push::{push_metrics, push_add_metrics, push_collector, push_add_collector, |
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.
Not sure what the backwards compatibility desires are at this point in the library's life, but the reexport could be maintained.
use std::env; | ||
use std::time; | ||
use std::thread; | ||
mod bridge { |
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.
Instead of this module, it is probably easier to conditionally change main
:
#[cfg(not(feature = "push"))]
fn main() {
println!(r#"Please enable feature "push", try:
cargo run --features=push --example example_push"#);
}
#[cfg(feature = "push")]
fn main() {
// original main here
}
You could then also have #![cfg_attr(not(feature = "push"), allow(warnings))]
at the top of the file to disable warnings on the unused imports and lazy_static
and so on.
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.
Yeah, that is much easier.
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.
wow it's even nicer than I thought it would be after that change! :-)
* reexport push functions * suppress warnings on none-push build
ec7942e
to
6d458ba
Compare
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.
LGTM
PTAL @siddontang |
Do we need to update TiKV? |
LGTM |
PTAL @BusyJay |
@@ -15,6 +15,7 @@ addons: | |||
env: | |||
global: | |||
- RUSTFLAGS=--deny=warnings | |||
- FEATURES=push |
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.
Should test whether everything works fine when the features is disabled.
Tests with no features enabled
PTAL @BusyJay |
LGTM |
This PR introduces a new feature
push
. It allows building core instrumentation library without hyper and libc.There are a few API changes:modpush
is published.following functions are moved to modpush
.Close #84
PTAL @siddontang @kamalmarhubi