feat: Add client hints for requestmeta #1802
Conversation
This PR renames two functions: * `RawUserAgentInfo::new()` -> `RawUserAgentInfo::from_headers()` * Fix typo in `samplingresult_from_rules_and_proccessing_flag`. This is a separate PR to avoid too many diffs in #1802. #skip-changelog
|
|
||
| #[cfg(test)] | ||
| // TODO: Remove Dsn here? | ||
| pub fn new(dsn: relay_common::Dsn) -> Self { |
There was a problem hiding this comment.
moved to mod tests{}
There was a problem hiding this comment.
In general, I'd suggest to keep PRs focused on their scope. More small and focused PRs is better than a few big PRs.
…into feat/reqmetahints
relay-general/src/store/mod.rs
Outdated
| @@ -63,6 +64,8 @@ pub struct StoreConfig { | |||
|
|
|||
| /// The SDK's sample rate as communicated via envelope headers. | |||
| pub client_sample_rate: Option<f64>, | |||
|
|
|||
| pub client_hints: ClientHints<String>, | |||
There was a problem hiding this comment.
Added clienthints as a separate field due to backwards compatibility wrt serialization. If this is not necessary for StoreConfig, let me know, and i'll instead use RawUserAgentInfo here.
| let mut ua = RawUserAgentInfo::default(); | ||
|
|
||
| for (key, value) in request.headers() { | ||
| ua.extract_header(key.as_str(), value.to_str().ok().map(str::to_string)); |
There was a problem hiding this comment.
This solution is a bit inelegant, but since RawUserAgentInfo is defined in another crate I would have had to import Request library in General to make this a method of RawUserAgentInfo. Since the other Header type is defined by us, we could perhaps implement From<HttpRequest> to make it prettier.
| #[serde(default, skip_serializing_if = "ClientHints::is_empty")] | ||
| client_hints: ClientHints<String>, |
There was a problem hiding this comment.
I'd move this a bit upwards and place it next to user_agent.
There was a problem hiding this comment.
right, I was worried there would be serialization issues, but i guess with json it doesnt matter
|
|
||
| #[cfg(test)] | ||
| // TODO: Remove Dsn here? | ||
| pub fn new(dsn: relay_common::Dsn) -> Self { |
There was a problem hiding this comment.
In general, I'd suggest to keep PRs focused on their scope. More small and focused PRs is better than a few big PRs.
| user_agent: Some("sentry/agent".to_string()), | ||
| no_cache: false, | ||
| start_time: Instant::now(), | ||
| client_hints: ClientHints::default(), |
There was a problem hiding this comment.
Could you please add tests which use non-default client_hints? To find out what tests would be affected, change to a non-default value here and see if any tests break.
Co-authored-by: Joris Bayer <joris.bayer@sentry.io>
…into feat/reqmetahints
* master: feat: Add client hints for requestmeta (#1802)
Second PR about supporting client hints. #1183
previous PR: #1752
This PR adds support for client hints on the requestmeta struct, which means that the client hints can be sent as part of an event json as an optional field. This means that more information will be extracted from the client hints rather than the user agent string, as relay will first attempt to check the client hints and only falling back to the user agent string if no client hints are found.