Conversation
…p, return other headers so coprocessor requests still work
This comment has been minimized.
This comment has been minimized.
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 2 changed, 0 removedBuild ID: 69ac644e3da468cee087b581 URL: https://www.apollographql.com/docs/deploy-preview/69ac644e3da468cee087b581 |
aaronArinder
left a comment
aaronArinder
left a comment
There was a problem hiding this comment.
only suggestion is some potentially different wording for the warn!()
| output.entry(k).or_insert_with(Vec::new).push(v) | ||
| match String::from_utf8(v.as_bytes().to_vec()) { | ||
| Ok(v) => output.entry(k).or_insert_with(Vec::new).push(v), | ||
| Err(e) => tracing::warn!("unable to externalize header value for {}: {}", k, e), |
There was a problem hiding this comment.
what does externalize mean? convertible to something... external? wondering if this might confuse folks
There was a problem hiding this comment.
Agreed, is this better? More direct
| Err(e) => tracing::warn!("unable to externalize header value for {}: {}", k, e), | |
| Err(e) => tracing::warn!("unable to convert header value to utf-8 for {}: {}", k, e), |
So the full error will look like unable to convert header value to utf-8 for test-header: invalid utf-8 sequence of 1 bytes from index 7
There was a problem hiding this comment.
that lgtm; and, I liked your idea of being explicit too by calling out that it won't be sent to the coprocessor (assuming this codepath only deals with coprocessors, if not, it's probably more effort than it's worth to add a log for just coprocessors)
There was a problem hiding this comment.
Pushed a commit that mentions it wont be sent to a coprocessor, currently the external mod is only used by coprocessor logic despite being separate, I think its reasonable that if it is used elsewhere in future then the implementor can change the log text
pragl
left a comment
pragl
left a comment
There was a problem hiding this comment.
I believe the docs change needs to be copied to one other location. Please take a look when you get a chance. Thank you!
…rough externalize_header_map (#8828)
…rough externalize_header_map (#8828)
3 participants
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
A lot of (if not most) features benefit from built-in observability and
debug-level logs. Please read this guidance on metrics best-practices. ↩Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩