-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add init #1
add init #1
Conversation
BenjaminAbt
commented
May 15, 2021
•
edited
Loading
edited
- init
- browsers agents
- bots agents
- tools agents
- nuget
- docs
- ci/cd
- tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻 definitely a good starting-point, good architecture, and what I've seen it shouldn't allocate too much* 😃
Review comments got a bit longer, though / sorry.
.editorconfig should be added and these rules run through code cleanup (end of file line endings).
So I expect this is the most performant solution we can use, should go forward with it -- here I'm pretty confident 🚀
What I don't like, and at the moment I have no better solution, is that the HttpUserAgentParser
needs to iterate and test (~ brute force). But this is simple and clean code which together with the CachedHttpUserAgentParserProvider
shouldn't be executed that often, as the majority of UAs should be served from the cache.
* when used with the cache it should be allocation-free (iif cache-hit), that's something I like very much!
The rest of this comment are some thoughts, but these won't hinder any progress of this PR.
How will the data in HttpUserAgentStatics
be updated / kept up-to-date?
I have no knowledge on how often UAs change or new bots come into play. But maybe we should have some sort of generator that creates the entries for us. Or we load at runtime (maybe periodically) UA strings and add them to the dictionary/list/array.
You know I love metrics and counters, so (in the future) we should add some counters for cache hit / miss and write the raw UA strings as events. So we can gather data for AI and analyze it.
So we could also detect new bots, etc. by an alert from AI and update the list of kwown UAs.
src/MyCSharp.HttpUserAgentParser.AspNetCore/HttpUserAgentParserAccessor.cs
Outdated
Show resolved
Hide resolved
src/MyCSharp.HttpUserAgentParser.AspNetCore/MyCSharp.HttpUserAgentParser.AspNetCore.csproj
Outdated
Show resolved
Hide resolved
src/MyCSharp.HttpUserAgentParser.AspNetCore/MyCSharp.HttpUserAgentParser.AspNetCore.csproj
Outdated
Show resolved
Hide resolved
src/MyCSharp.HttpUserAgentParser.MemoryCache/MyCSharp.HttpUserAgentParser.MemoryCache.csproj
Show resolved
Hide resolved
src/MyCSharp.HttpUserAgentParser/DependencyInjection/ServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Günther Foidl <[email protected]>
src/MyCSharp.HttpUserAgentParser.MemoryCache/HttpUserAgentParserMemoryCachedProviderOptions.cs
Show resolved
Hide resolved
src/MyCSharp.HttpUserAgentParser/Providers/DefaultHttpUserAgentParserProvider.cs
Outdated
Show resolved
Hide resolved
src/MyCSharp.HttpUserAgentParser.MemoryCache/HttpUserAgentParserMemoryCachedProvider.cs
Outdated
Show resolved
Hide resolved
src/MyCSharp.HttpUserAgentParser.MemoryCache/HttpUserAgentParserMemoryCachedProvider.cs
Outdated
Show resolved
Hide resolved
src/MyCSharp.HttpUserAgentParser/Providers/HttpUserAgentParserCachedProvider.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.editorconfig for consistent newlines would be nice 😉
Modulo these points - LGTM.
You can bring the change in (once these are addressed).
Some other points (regex compiled, dictionary) we can address later, as it's no stopper for this PR / project.
src/MyCSharp.HttpUserAgentParser.AspNetCore/MyCSharp.HttpUserAgentParser.AspNetCore.csproj
Outdated
Show resolved
Hide resolved
new(CreateDefaultPlatformRegex("symbian"), "Symbian OS", HttpUserAgentPlatformType.Symbian), | ||
}; | ||
|
||
private const RegexOptions DefaultBrowserRegexFlags = RegexOptions.IgnoreCase | RegexOptions.Compiled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking about RegexOptions.Compiled
.
If these regexes are only called if the UA isn't in the cache, is it worth then to have compiled regexes?
Compiled regex have higher startup-cost (won't matter much for us, but matter e.g. for Azure Functions) and need to be kept around / alive for the lifetime of the app. They are only ever re-executed once the UA is evicted from the cache (e..g by size limit).
I don't know what's better here. For the moment we should leave it as is, and should re-evaluate that later (--> #3).
src/MyCSharp.HttpUserAgentParser/MyCSharp.HttpUserAgentParser.csproj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, I'm pedantic (sorry...).
...rp.HttpUserAgentParser.MemoryCache.UnitTests/HttpUserAgentParserMemoryCachedProviderTests.cs
Show resolved
Hide resolved
src/MyCSharp.HttpUserAgentParser.AspNetCore/HttpUserAgentParserAccessor.cs
Show resolved
Hide resolved
src/MyCSharp.HttpUserAgentParser.MemoryCache/HttpUserAgentParserMemoryCachedProvider.cs
Show resolved
Hide resolved
src/MyCSharp.HttpUserAgentParser.MemoryCache/HttpUserAgentParserMemoryCachedProvider.cs
Outdated
Show resolved
Hide resolved
src/MyCSharp.HttpUserAgentParser.MemoryCache/HttpUserAgentParserMemoryCachedProvider.cs
Outdated
Show resolved
Hide resolved
Lets bring the PR in, do the rest in further PRs to keep it manageable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd bring the benchmarks in as separate change.
A PR more won't harm the project, and the changes are separated cleaner.
We also have some follow up work to do.
Otherwise we have monster PRs with lots of comments and orthogonal change-sets.