Add resource detector #174
Conversation
TommyCpp
force-pushed
the
env
branch
2 times, most recently
from
42bda30 to
72f6074
Compare
Add import in tests mod.
See oteps#111 for details Fix lint.
According to the proposal. detect should accept a timeout parameter and return Resource regardless.
src/sdk/env.rs
Outdated
| @@ -0,0 +1,88 @@ | |||
| //! EnvResourceDetector | |||
| //! | |||
| //! Implementation of ResourceDetector in extract information from environment. | |||
There was a problem hiding this comment.
| //! Implementation of ResourceDetector in extract information from environment. | |
| //! Implementation of `ResourceDetector` to extract a `Resource` from environment variables. |
src/sdk/env.rs
Outdated
| const OTEL_RESOURCE_ATTRIBUTES: &str = "OTEL_RESOURCE_ATTRIBUTES"; | ||
|
|
||
| /// Resource detector implements ResourceDetector and is used to extractor general SDK configuration from environment | ||
| /// See https://github.com/open-telemetry/opentelemetry-specification/tree/master/specification/resource/semantic_conventions#telemetry-sdk for semantic conventions. |
There was a problem hiding this comment.
Can we wrap these to 80 characters? rustfmt currently doesn't do this automatically, but the style guide for comments in rust seems to be 80 currently https://github.com/rust-dev-tools/fmt-rfcs/blob/master/guide/guide.md#comments
There was a problem hiding this comment.
Sure thing. Don't think I can break the url but will wrap other comments.
src/sdk/env.rs
Outdated
| /// Resource detector implements ResourceDetector and is used to extractor general SDK configuration from environment | ||
| /// See https://github.com/open-telemetry/opentelemetry-specification/tree/master/specification/resource/semantic_conventions#telemetry-sdk for semantic conventions. | ||
| #[derive(Debug)] | ||
| pub struct EnvResourceDetector {} |
There was a problem hiding this comment.
we've been using _private here so we could add more without breaking changes (forces people to use the new method) e.g.
opentelemetry-rust/src/api/correlation/propagation.rs
Lines 17 to 19 in e0bc364
src/sdk/env.rs
Outdated
| /// Extract key value pairs and construct a resource from resources string like key1=value1,key2=value2,... | ||
| fn construct_otel_resources(s: String) -> Resource { | ||
| let mut key_values = vec![]; | ||
| for entries in s.split(',') { |
There was a problem hiding this comment.
split_terminator here may be preferable as it will have the behavior people probably expect form trailing commas
There was a problem hiding this comment.
Sounds reasonable. Thanks!
src/sdk/resource.rs
Outdated
|
|
||
| /// Describes an entity about which identifying information and metadata is exposed. | ||
| /// | ||
| /// Items are sorted by key, and are only overwritten if the value is an empty string. | ||
| /// Items are sorted by keys, and are only overwritten if the value is an empty string. |
There was a problem hiding this comment.
Sorted by keys is also a little awkward, maybe "sorted by their key"?
See PR discussion for details.
|
@TommyCpp I think you can merge your own PRs once approved now 👍 |
|
@jtescher I tried but I don't think I can. Maybe you will have to be maintainer to do that? |
|
Hm maybe, the permissions in the otel org seem a little overly complicated to me. I'll just merge this for now 🤷♂️ |
Fix #167
See open-telemetry/oteps#111 for proposal
Also fixes the bug in test mod of
Resource