-
Notifications
You must be signed in to change notification settings - Fork 451
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
feat: add methodfrom_env
to prometheus exporter builder
#605
Conversation
This is my first attempt at addressing #242. I'm new to Rust so any suggestions on better approaches are much appreciated. Also, hopefully this aligns with the direction this issues was looking to address. I have switched it to Draft because I need to get the CLA addressed later tonight when the kids are off to bed :). |
I think the CLA issue is because you didn't link your email in commit to your GitHub account. |
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.
Looks like a good start, thanks!
It may be worth updating the hyper-prometheus
example to demonstrate users can use host
and port
information to start the webserver on the desired port.
The next step would be to try to solve #293 and use the host/port configuration in the default webserver
#[test] | ||
fn test_from_env() { | ||
let otel_exporter_prometheus_host = "OTEL_EXPORTER_PROMETHEUS_HOST"; | ||
let otel_exporter_prometheus_port = "OTEL_EXPORTER_PROMETHEUS_PORT"; | ||
|
||
// environment variables do not exist | ||
env::remove_var(otel_exporter_prometheus_host); | ||
env::remove_var(otel_exporter_prometheus_port); | ||
let exporter = opentelemetry_prometheus::ExporterBuilder::from_env().init(); | ||
assert_eq!(exporter.host(), "0.0.0.0"); | ||
assert_eq!(exporter.port(), "9464"); | ||
|
||
// environment variables are available and non-empty strings | ||
env::set_var(otel_exporter_prometheus_host, "prometheus-test"); | ||
env::set_var(otel_exporter_prometheus_port, "9000"); | ||
|
||
let exporter = opentelemetry_prometheus::ExporterBuilder::from_env().init(); | ||
assert_eq!(exporter.host(), "prometheus-test"); | ||
assert_eq!(exporter.port(), "9000"); | ||
|
||
// environment variables are available and empty | ||
env::set_var(otel_exporter_prometheus_host, ""); | ||
env::set_var(otel_exporter_prometheus_port, ""); | ||
let exporter = opentelemetry_prometheus::ExporterBuilder::from_env().init(); | ||
assert_eq!(exporter.host(), "0.0.0.0"); | ||
assert_eq!(exporter.port(), "9464"); | ||
} |
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.
This is probably more suitable to be in lib.rs
as it is a unit test.
We can have a new mod in lib.rs
with #[cfg(test)]
and leave this part there.
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.
I had it there at first and then moved it into the integration test to stay consistent. I will move it back.
opentelemetry-prometheus/src/lib.rs
Outdated
pub fn from_env() -> Self { | ||
let mut builder = ExporterBuilder::default(); | ||
|
||
if let Some(host) = env::var("OTEL_EXPORTER_PROMETHEUS_HOST") | ||
.ok() | ||
.filter(|s| !s.is_empty()) | ||
{ | ||
builder = builder.with_host(host) | ||
} | ||
|
||
if let Some(port) = env::var("OTEL_EXPORTER_PROMETHEUS_PORT") | ||
.ok() | ||
.filter(|s| !s.is_empty()) | ||
{ | ||
builder = builder.with_port(port); | ||
} | ||
|
||
builder | ||
} | ||
|
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.
I think we decided a while ago to try to fetch env vars in default
method. See #459 for details.
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.
Ahh...I didn't see that and was going off from the old comment in the issue. I will go ahead and implement it in the default.
Also, It looks like the OTEL_RESOURCE_ATTRIBUTES
was added to the Default implementation of Resource so I don't think I need to do anything special for the prometheus exporter.
Codecov Report
@@ Coverage Diff @@
## main #605 +/- ##
=====================================
Coverage 54.7% 54.7%
=====================================
Files 101 101
Lines 8945 8943 -2
=====================================
Hits 4897 4897
+ Misses 4048 4046 -2
Continue to review full report at Codecov.
|
Thanks for the review. I will address the feedback tomorrow.
Yes, thanks I need to get me correct email linked.
I will take a look at this too and see if I can get it updated. |
It looks like a standard prometheus pull based scenario where hyper is setup with an endpoint for scraping. This to me aligns with case 2 in the issue where it is a middleware in a web framework. In that scenario, I don't think these would get used but rather they would be used for either case 1 (push) or case 3 (non-existing web server). Granted since it is part of the public api of So, I think updating or including a new example once #293 lands is probably more appropriate, thoughts? |
I have addressed feedback and left comments. I also signed the CLA and got my user-id added to the new commit. It looks like the initial commit is still not associated to my user id. I can squash the commit and clean-up the commit message but I didn't want to confuse the review process. Let me know how you want to proceed! Cheers, |
Yeah, I think you need to do a squash or something like that, feel free to do so. The changes look good. I Will take a closer look at them and the comments later. |
Sounds reasonable to me 👍 We may also consider include those use cases with Also looks like there is a new version of Clippy with new lint rules. Could you help fix those also? Thanks in advance! |
Fixes open-telemetry#293 Allows configuring the `PrometheusExporter` using environment variables defined in the semantic conventions of the Otel specification. Currently, supports setting the host and port for prometheus in the builder and will fall back to the defaults defined in the otel specification.
Alright, I amended the commit with the missing author. Looks like all the CLA stuff is good to go now. I have updated to rust 1.54, fixed all the new clippy lint issues and rebased so CI should be green now. I have left all the individual commits in so that I didn't mess up the PR review history. Does this project squash and cleanup the commit messages before merging into master or would you rather I squash them? |
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.
Awesome work! We do squash and merge on PRs but feel free to squash it now if you want to.
Just one last suggestion and we should be good to go.
opentelemetry-prometheus/src/lib.rs
Outdated
/// The port used by the prometheus exporter | ||
/// | ||
/// If not set it will be defaulted to port "9464" | ||
port: Option<String>, |
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.
I wonder if it will make sense to use an unsigned int here?
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, I think that makes sense. I did a quick look around to see how the various frameworks and standard library handle it.
std::net::SocketAddr
treats port as u16
- https://doc.rust-lang.org/std/net/enum.SocketAddr.html#method.port
std::net::ToSocketAddr
trait also shows most examples treating it as u16
- https://doc.rust-lang.org/std/net/trait.ToSocketAddrs.html
If I look at a few Web Frameworks they are u16
becuase they ultimately accept the SocketAddr.
- https://actix.rs/ - leverages a tuple with the port treated like a u16 for conversion to SocketAdd
- https://rocket.rs/v0.5-rc/guide/configuration/#overview
- https://github.com/hyperium/tonic/blob/master/tonic/src/transport/server/mod.rs#L622
- https://docs.rs/hyper/0.14.11/hyper/server/index.html
I can make the switch but I guess the only question is that when parsing the env variable in the impl for default
how would we handle the potential parse error? I guess it would just get ignored and we fall back to default port? The only problem with that is we are not giving the user any feedback that it occurred so would panic early be a better dev experience?
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.
That's a good question. I think a good approach is to fall back to the default one and print an error message using global::handle_error
function.
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.
Cool, I have gone ahead and made the change and logged an error message.
An error is logged to the global OpenTelemetry error handling when we fail to parse the port from an OS Environment variable
I'm not sure what happened with the CLA? I haven't changed anything and I looked and all the commits are associated to the correct email address. |
Oh, I think CLA is down for now. Gonna have to wait for it to come back. |
/easycla |
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, good work!
Fixes #242
Allows configuring the
PrometheusExporter
using environment variables defined in the semantic conventions of the Otel specification. Currently, supports setting the host and port for prometheus in the builder and will fall back to the defaults defined in the otel specification.