[WIP] - [Metrics ServerApp] - New metrics serverapp #1689
Conversation
|
Could you send met a link / zip to the database dump you used and I can test it? |
manio143
left a comment
manio143
left a comment
There was a problem hiding this comment.
Great work! Left some suggestions.
| public IEnumerable<ProjectsUsersAggregation> GetProjectsUsers() | ||
| { | ||
| var result = _metricDbContext.MetricEvents | ||
| .Where(a => a.MetricId == 19) |
There was a problem hiding this comment.
Can you put that 19 in a names constant and explain which metric it represents (like const int XxxMetricId = 19;)?
| g.Sum(x => x.Count > 3 ? 1 : 0), | ||
| g.Sum(x => x.Count > 5 ? 1 : 0) | ||
| )) | ||
| .ToList(); |
There was a problem hiding this comment.
nit: ToListAsync() like below - applies in other methods as well
| /// </summary> | ||
| /// <returns>The installs count in the last 30 days per day</returns> | ||
| [HttpGet] | ||
| [Route("installs-last-days")] |
There was a problem hiding this comment.
nit: route path can be moved up into the [HttpGet]
| return Ok(); | ||
| } | ||
|
|
||
| var clock = Stopwatch.StartNew(); |
There was a problem hiding this comment.
Do we need to stop the Stopwatch before exiting this method?
| Timestamp = overrideTime ?? default | ||
| }; | ||
|
|
||
| var metricAsJson = JsonConvert.SerializeObject(newMetric); |
There was a problem hiding this comment.
Would it make sense to move this serialization into the if block - this way we don't do it unnecessarily on the happy path.
| builder.Services.AddDbContext<MetricDbContext>(opt => | ||
| { | ||
| opt.UseSqlServer(builder.Configuration.GetConnectionString("DefaultConnection"), | ||
| options => options.CommandTimeout(999999999)); |
There was a problem hiding this comment.
I would strongly suggest making this smaller 😄
15 minutes timeout should probably be enough - it's not like people will be waiting that long before getting discouraged anyways and maybe the query needs to be further optimized or db upgraded if it takes that long...
There was a problem hiding this comment.
Haha, yes, that was intended, just for testing purposes as some queries are very time consuming. I think I will remove it completely and try to cache some queries using materialized views or additonal table
There was a problem hiding this comment.
Regarding the caching, from .NET 7 you can cache very easily through minimal API, or a controller through OutputCache, maybe you know about that.
https://learn.microsoft.com/en-us/aspnet/core/performance/caching/output?view=aspnetcore-7.0
| ## Prerequisites | ||
|
|
||
| - `ASPNET_ENVIRONMENT`: Set this variable to `Development` if you want to run the application in a testing environment. In the development environment, the application will connect to a non-Local SQL Server, allowing you to run the application on any operating system (i.e. [by using docker image](https://hub.docker.com/_/microsoft-mssql-server).). | ||
| - `--seed-metrics` (recommended): Set this argument if you want to seed the database with initial data. |
There was a problem hiding this comment.
I don't see this in Program.cs
|
I am no longer working this PR, see #2261 |
3 participants
PR Details
Description
PR migrates current ASP.NET project to ASP.NET Core + EntityFramework/Dapper. I've also rearranged code refactored code to make it more readable. Additionally, project has Swagger support to easily call endpoints.
Types of changes
Todo
ActivityMetrics.Controller. GetCrashesPerVersion- takes about 2 minutes to load. I was not able to materialize the query. Probably need to create aMetricCachetable, create a job using i.e HangFire lib or SQL agentActivityMetrics.Controller.GetHighUsage- I need to test this, sometimes it takes too long to load.I am leaving this PR as a WIP because I need a break. Though if you want to help, please feel free to collaborate and contribute your ideas or suggestions through my fork or anywhere😊