Fix sidecar logging #521
Merged
Fix sidecar logging #521
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BenchmarksComparisonParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. CandidateCandidate benchmark detailsGroup 1
WarningsBaselineBaseline benchmark detailsGroup 1
Warnings |
cataphract
force-pushed
the
glopes/fix-sidecar-logging
branch
2 times, most recently
from
eac99b3
to
4b6ff40
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #521 +/- ##
==========================================
- Coverage 70.46% 70.46% -0.01%
==========================================
Files 205 205
Lines 27939 27936 -3
==========================================
- Hits 19688 19685 -3
Misses 8251 8251
|
sidecar/src/config.rs
Outdated
Comment on lines
132
to
133
| method if method.starts_with("file:///") => { | ||
| let fixed_uri = format!("file://./{}", &method[7..]); | ||
| Uri::from_str(&fixed_uri) | ||
| .map(|u| LogMethod::File(PathBuf::from(&u.path()))) | ||
| .unwrap_or_default() | ||
| } |
There was a problem hiding this comment.
Why not just this instead of the roundtrip to url?
Suggested change
| method if method.starts_with("file:///") => { | |
| let fixed_uri = format!("file://./{}", &method[7..]); | |
| Uri::from_str(&fixed_uri) | |
| .map(|u| LogMethod::File(PathBuf::from(&u.path()))) | |
| .unwrap_or_default() | |
| } | |
| method if method.starts_with("file://") => { | |
| let path = method.strip_prefix("file://").unwrap(); | |
| LogMethod::File(PathBuf::from(path)) | |
| } |
There was a problem hiding this comment.
It's because the specification for the path is in the form file:///my/path. However, even though this is a valid URI (with an empty host), see RFC 3986:
URI = scheme ":" hier-part [ "?" query ] [ "#" fragment ]
hier-part = "//" authority path-abempty
/ path-absolute
/ path-rootless
/ path-empty
authority = [ userinfo "@" ] host [ ":" port ]
host = IP-literal / IPv4address / reg-name
reg-name = *( unreserved / pct-encoded / sub-delims )
If the URI scheme defines a default for host, then that default
applies when the host subcomponent is undefined or when the
registered name is empty (zero length). For example, the "file" URI
scheme is defined so that no authority, an empty host, and
"localhost" all mean the end-user's machine, whereas the "http"
scheme considers a missing authority or empty host invalid.
the http package rejects the uri for not having a host. So add a bogus host (.).
There was a problem hiding this comment.
Going through the Uri struct is not a good idea. If there are non-alphanumeric characters, they would need to be URL-encoded for this to work. This value comes from an env variable that can be manually supplied so encoding is painful to do.
We already do not follow the URI standard for unix sockets URL (since the logic is just unix:// + path on disk without any special encoding ) so I don't see the point of arbitrarily following it for this.
There was a problem hiding this comment.
ah sorry I didn't read that right. I can't just strip the prefix because it may have urlencoded characters
cataphract
force-pushed
the
glopes/fix-sidecar-logging
branch
from
4b6ff40
to
1c53282
Compare
Broken by ce2afd9, since when parse_uri starts returning a bogus uri, with the path put in the authority in hexadecimal form and the actual path empty. Sidestep that mess by not using parse_uri.
cataphract
force-pushed
the
glopes/fix-sidecar-logging
branch
from
1c53282
to
108f9fa
Compare
paullegranddc
approved these changes
Jul 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Broken by ce2afd9, since when parse_uri starts returning a bogus uri, with the path put in the authority in hexadecimal form and the actual path empty. Sidestep that mess by not using parse_uri.