Closed kriswuollett closed 11 months ago
Can you use the Key::as_str()
method in this context? Maybe if we just made that method const
?
I forked the repo and used my local path to make these modifications in opentelemetry-rust
:
/// Returns a reference to the underlying key name
pub const fn as_str(&self) -> &str {
self.0.as_str()
}
impl OtelString {
const fn as_str(&self) -> &str {
match self {
OtelString::Owned(s) => panic!(),
OtelString::Static(s) => s,
OtelString::RefCounted(s) => panic!(),
}
}
}
The above panic!()
in there is just to get it to compile due to:
error[E0015]: cannot call non-const fn `<Box<str> as AsRef<str>>::as_ref` in constant functions
--> opentelemetry/src/common.rs:148:39
|
148 | OtelString::Owned(s) => s.as_ref(),
| ^^^^^^^^
|
= note: calls in constant functions are limited to constant functions, tuple structs and tuple variants
Even with the changes compiled the following statement:
event!(Level::INFO, { semconv::trace::CODE_FUNCTION.as_str() } = "do_work", "hello_world");
error[E0716]: temporary value dropped while borrowed
--> src/main.rs:12:2
|
12 | event!(Level::INFO, { semconv::trace::CODE_FUNCTION.as_str() } = "do_work", "Initialized telemetry");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| creates a temporary value which is freed while still in use
| temporary value is freed at the end of this statement
| cast requires that borrow lasts for \`'static\`
|
= note: this error originates in the macro \`$crate::fieldset\` which comes from the expansion of the macro \`event\` (in Nightly builds, run with -Z macro-backtrace for more info)
Even if that worked I'd probably protest a bit because of the copy-and-pasted .as_str()
calls. :-)
Please see the following example to test usage. Its output should look something like:
% cargo run
Compiling tracing-example v0.1.0 (/Users/kris/Code/kriswuollett/tracing-example)
Finished dev [unoptimized + debuginfo] target(s) in 0.54s
Running \`target/debug/tracing-example\`
2023-10-26T11:45:01.386046Z INFO main ThreadId(01) tracing_example: Initialized telemetry
2023-10-26T11:45:01.386230Z INFO main ThreadId(01) do_work: tracing_example: Preparing to print greeting to console code.function="do_work" com.tracing-example.user-name="Anonymous" code.function="do_work"
Hello, Anonymous!
[package]
name = "tracing-example"
version = "0.1.0"
edition = "2021"
[dependencies]
opentelemetry-semantic-conventions = { version = "0.12.0", default-features = false }
tracing = { version = "0.1.37", default-features = false }
tracing-subscriber = { version = "0.3.17", default-features = false, features = ["fmt"] }
use tracing::{event, span, Level};
use tracing_subscriber::{prelude::*};
pub mod semconv {
pub const CODE_FUNCTION: &str = "code.function";
}
pub mod api_methods {
pub const DO_WORK_METHOD: &str = "do_work";
}
pub mod api_attrs {
pub const USER_NAME: &str = "com.tracing-example.user-name";
}
fn do_work(user_name: &str) {
let span = span!(
Level::INFO,
api_methods::DO_WORK_METHOD,
{ semconv::CODE_FUNCTION } = api_methods::DO_WORK_METHOD
);
let _enter = span.enter();
event!(
Level::INFO,
{ semconv::CODE_FUNCTION } = api_methods::DO_WORK_METHOD,
{ api_attrs::USER_NAME } = user_name,
"Preparing to print greeting to console"
);
println!("Hello, {user_name}!");
}
fn main() {
let fmt = tracing_subscriber::fmt::layer()
.with_level(true)
.with_target(true)
.with_thread_ids(true)
.with_thread_names(true)
.with_ansi(false)
.compact();
tracing_subscriber::registry().with(fmt).init();
event!(Level::INFO, "Initialized telemetry");
do_work("Anonymous");
}
I'd be in favor of changing this and all the others from:
pub const CODE_FUNCTION: Key = Key::from_static_str("code.function");
to
pub const CODE_FUNCTION = "code.function";
and relying on the existing const fn Key::from_static_str
and impl From<&'static str> for Key
to compose the rest where needed.
I see no fundamental reason why the semantic convention consts need to be bound to Key
.
If other @open-telemetry/rust-approvers are not opposed, would @kriswuollett be up for submitting a PR?
I think it's a valid use cases.
I see no fundamental reason why the semantic convention consts need to be bound to Key
I think it's easier for users to just import a Key and attach it but if it's blocking other use case we don't have to maintain this behavior. A simple static string maybe a better choice
@shaun-cox, yes I'd be happy to put together a PR for this.
Just to confirm, these changes are solely applicable to the semantic conventions of traces and do not extend to resources, correct?
Just to confirm, these changes are solely applicable to the semantic conventions of traces and do not extend to resources, correct?
No. Although the title of the issue says tracing
, I did intend for it to mean any "key" that one would use in the fields for a library like the tracing
crate and its span!
and event!
macros. I most definitely would want to use things like SERVICE_NAME
too.
I think for something like this we'd need 2 PRs? One to add/deprecate w/ experimental feature to remove, and the second that removes the experimental feature and just removes deprecated definitions. I'll introduce myself on the community chat channel.
I'm not a fan of any approach where we end up with two sets of constants. I think it's better to have whole bunch of &'static str
consts and then make the path to build Key
s from that as smooth as possible.
I'm not a fan of any approach where we end up with two sets of constants. I think it's better to have whole bunch of &'static str consts and then make the path to build Keys from that as smooth as possible.
+1
@djc, so the project is okay with a potentially breaking change where all the Key
s are just directly replaced by &'static str
in #1334?
Yes, we generally don't try very hard to keep compatibility.
Yes, we generally don't try very hard to keep compatibility.
We're attempting to get to stable, but for now breaking changes more need to be noted in the Changelog for users to know.
I'd like to do something like the following with
tracing
:The
tracing
crate requires values that look like Rust identifiers, string literals, or constant expressions. But the string for values like code function are wrapped up in aKey
which althoughconst
does not satisfy theformat_args
that is used bytracing
:I'd think that the easiest solution would be to create another submodule that is code generated with the just primary source of the key strings from which the module with the
const Key
s are derived, e.g.,:That way something like the above example would work for
tracing
as well as any other context in whichconst
/&'static str
is required.Although the code generation can be on my end, automatically or manually, this seems basic enough and useful to others that I'd think it should be implemented here? Or is there some Rust technique or library I haven't learned of yet to make use of the existing constants instead of copy and pasting raw
str
values like"code.function"
everywhere?