Removed openSSL requirement & fixed histogram issues#223
Open
Descent098 wants to merge 2 commits intofcsonline:masterfrom
Open
Removed openSSL requirement & fixed histogram issues#223Descent098 wants to merge 2 commits intofcsonline:masterfrom
Descent098 wants to merge 2 commits intofcsonline:masterfrom
Conversation
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Warning: I don't write rust, I just made changes until everything worked for me
Apologies this is 2 changes in 1, I wasn't planning on contributing these, but I figured I would leave that up to the maintainer not me.
Reqwest
I updated the
reqwestdependency to0.13.2, which removes the need for openSSL entirely (#124 ). I wasn't fully sure whattrust-dnsdid in the older versions of request, but I went through the changelogs and found that It was removed in this pr and replaced withsystem-proxy, so I tacked that on there.I don't know the rust ecosystem so I ran
cargo update, which seems to have also updated other dependencies in the lockfile, but no actual usability seems to be affected by this.Histogram
The histogram was broken for me (and some others #216 #174 #151 #201), which caused panics. It happened on any requests exceeding
3.6s, so I think the units might have been off. I set the new timeout quite high (think it works out to an hour, which looks like that's what was intended), so that can be tweaked if need be (getting rid of one 60 should be 1 min I think).Testing
I couldn't see any tests, or know how to run them if there are any, so the only tests I ran were the tests I was running for a project at work. Everything works, and comparing this version to the baseline nothing seems to have broken, and all the numbers are the same (within ~1-5ms).