Repository navigation
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
9034729 to
472c8f9
Compare
richardpringle
left a comment
richardpringle
left a comment
There was a problem hiding this comment.
I left some comments that aren't blockers... but the more we write code like this, the more work it is to undo it. I would suggest we land this then once verified, you spend the time to unwrap some of these coupled free functions.
I can't approve yet as there's real dependency addition here
| /// known-defective releases with an upgrade prompt. | ||
| const TVC_CLIENT_VERSION_HEADER: &str = "X-TVC-CLIENT-VERSION"; | ||
| const TRACEPARENT_HEADER: HeaderName = HeaderName::from_static("traceparent"); | ||
| const TRACESTATE_HEADER: HeaderName = HeaderName::from_static("tracestate"); |
| enum BoundedHeaderValueError { | ||
| TooLong, | ||
| Invalid, | ||
| } |
| } | ||
| } | ||
|
|
||
| type TraceContextHeaderValue = BoundedHeaderValue<MAX_TRACE_CONTEXT_HEADER_LEN>; |
| if headers | ||
| .get(TRACESTATE_HEADER) | ||
| .is_some_and(|value| value.is_empty()) |
There was a problem hiding this comment.
Make sure tracestate isn't empty before you get to this point with let tracestate = tracestate.filter(|val| !val.is_empty())
There was a problem hiding this comment.
applied.
didn't apply in normalize_trace_context_headers() because the W3C propagator will inject a tracestate automatically for
- No input tracestate
- Malformed non-empty tracestate
- Whitespace-only tracestate
which we want to avoid
| #[instrument(skip_all)] | ||
| pub(crate) fn build_turnkey_client( | ||
| stamper: TurnkeyP256ApiKey, | ||
| api_base_url: &str, | ||
| ) -> Result<TurnkeyClient<TurnkeyP256ApiKey>> { | ||
| let traceparent = read_env_var(ENV_TRACEPARENT); | ||
| let tracestate = read_env_var(ENV_TRACESTATE); | ||
|
|
||
| build_turnkey_client_with_trace_context( | ||
| stamper, | ||
| api_base_url, | ||
| traceparent.as_deref(), | ||
| tracestate.as_deref(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Same comment as above. This is an anti-pattern and couples code.
We want to separate resource acquisition from business logic. The function should receive ready-to-use dependencies, so it will have fewer failure modes, no hidden setup or I/O, less coupling to a concrete construction method, and is easier to test and reuse. Failures also stay at the wiring boundary, where they can be handled with better context.
There was a problem hiding this comment.
updated to
pub(crate) fn build_turnkey_client(
stamper: TurnkeyP256ApiKey,
api_base_url: &str,
headers: HeaderMap,
) -> Result<TurnkeyClient<TurnkeyP256ApiKey>>
let me know if that's what you meant
472c8f9 to
454a932
Compare
bfad827 to
49d50bf
Compare
- support `traceparent` and `tracestate` headers - Using official OTEL propagator
49d50bf to
3beeac8
Compare
|
self-blocking on merging until I resolve a discussion about how we do backend tracing and resolve |
|
@f3nry 's work removed the need for this |
2 participants
Summary
Supports TVC-265 and tkhq/mono#7539.