-
Notifications
You must be signed in to change notification settings - Fork 502
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
[propagation] Add support for baggage metadata/prop. #287
Conversation
W3 baggage spec allows users to pass properties along with baggage. This commit introduces metadata into `KeyValueMetadata` type. Propagators that supports baggage metadata could use it to propagate data. For example, user can use `insert_with_metadata` to add an entry into the baggage. ``` let mut cc = Baggage::new(); let _ = cc.insert_with_metadata("my-name", "my-value", "test"); ``` Or if the propagation the users uses doesn't support metadata. Users can also use `insert` ``` let mut cc = Baggage::new(); let _ = cc.insert_with("my-name", "my-value"); ```. Internally metadata is just an opaque string wrapper with no semantic meanings. We will assign a metadata for every entry in baggage. Note that since the Metadata currently has no semantic meanings. We will just take the metadata string from header without any validation and process.
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.
The logic looks good. Just left a few suggestions about the API.
5ca0ff6
to
cf4b7ca
Compare
src/api/baggage.rs
Outdated
/// let mut cc = Baggage::new(); | ||
/// let _ = cc.insert_with_metadata("my-name", "my-value", "test"); | ||
/// | ||
/// assert_eq!(cc.get_with_metadata("my-name"), Some(&(Value::String("my-value".to_string()), "test".into()))) |
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.
Same
src/sdk/propagation/baggage.rs
Outdated
.map(|(name, value)| { | ||
.map(|(name, (value, metadata))| { | ||
let metadata_str = if metadata.as_str().trim().is_empty() { | ||
"".to_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.
this can probably have metadata_str
be a str instead of string, don't need either string conversion or the as str below
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.
Hmm I think in else
caluse, the format!(";{}"...)
cannot be stored as &str
since it will be freed at the end clause if metadata_str
doesn't own it.
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.
oh right you need the prefix. What about something like:
let metadata_str = metadata.as_str().trim();
let metadata_prefix = if metadata_str.is_empty() { "" } else { ";" };
utf8_percent_encode(name.as_str().trim(), FRAGMENT)
.chain(iter::once("="))
.chain(utf8_percent_encode(String::from(value).trim(), FRAGMENT))
.chain(iter::once(metadata_prefix))
.chain(iter::once(metadata_str))
.collect()
})
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.
Great idea 👍
be61712
to
fa71d2f
Compare
@frigus02 any other thoughts or is this good to merge? |
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 great. Found one line of commented code. 🙂
…try-rust into baggage-metadata � Conflicts: � src/sdk/propagation/baggage.rs
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.
Thanks @TommyCpp
W3 baggage spec allows users to pass properties along with baggage. This commit introduces metadata into
KeyValueMetadata
type. Propagators that supports baggage metadata could use it to propagate data.For example, user can use
insert_with_metadata
to add an entry into the baggage.Or if the propagation the users uses doesn't support metadata. Users can also use
insert
Internally metadata is just an opaque string wrapper with no semantic meanings.
We will assign a metadata for every entry in baggage.
Note that since the Metadata currently has no semantic meanings. We will just take the metadata string from header without any validation and process.
Closes #260