Skip to content

Implement dashboard log query for PostgreSQL provider. - #180

Open
mo-esmp wants to merge 6 commits into
masterfrom
feature/#153-implement-postgresql-dashboard-query
Open

Implement dashboard log query for PostgreSQL provider.#180
mo-esmp wants to merge 6 commits into
masterfrom
feature/#153-implement-postgresql-dashboard-query

Conversation

@mo-esmp

@mo-esmp mo-esmp commented Aug 1, 2025

Copy link
Copy Markdown
Member

Introduced a new LogStatisticModel.cs class for structured dashboard statistics. Added comprehensive tests in DataProviderDashboardTests.cs to ensure correct functionality. Refactored various test files for better organization and clarity.

mo-esmp added 2 commits August 1, 2025 08:17
Improved handling of data providers in `AggregateDataProvider.cs` with added guard clauses and a new `SelectedDataProvider` property. Updated `IDataProvider.cs` to include a method for fetching dashboard statistics asynchronously. Streamlined log data fetching in `ElasticSearchDbDataProvider.cs`, `MongoDbDataProvider.cs`, and `SqlServerDataProvider.cs` for better readability and maintainability.

Introduced a new `DashboardModel` class in `LogStatisticModel.cs` for structured dashboard statistics. Added comprehensive tests in `DataProviderDashboardTests.cs` to ensure correct functionality. Refactored various test files for better organization and clarity.
@mo-esmp
mo-esmp requested review from Copilot and followynne August 1, 2025 06:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements dashboard log query functionality for PostgreSQL provider, adding the ability to fetch aggregated statistics for log data visualization. The changes introduce a new DashboardModel class for structured dashboard statistics and update the data provider interface to support dashboard queries.

  • Added FetchDashboardAsync method to IDataProvider interface for retrieving dashboard statistics
  • Implemented PostgreSQL-specific dashboard query functionality with log level aggregation and date filtering
  • Created comprehensive tests to ensure correct dashboard functionality across different PostgreSQL configurations

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Serilog.Ui.Core/Models/LogStatisticModel.cs Introduces new DashboardModel class with properties for dashboard statistics
src/Serilog.Ui.Core/IDataProvider.cs Adds FetchDashboardAsync method to the interface
src/Serilog.Ui.PostgreSqlProvider/PostgresDataProvider.cs Implements dashboard query functionality for PostgreSQL provider
tests/Serilog.Ui.PostgreSqlProvider.Tests/DataProvider/DataProviderDashboardTests.cs Comprehensive test suite for dashboard functionality
Various provider files Updates all existing providers to implement new interface method with NotImplementedException
Comments suppressed due to low confidence (4)

src/Serilog.Ui.Core/Models/LogStatisticModel.cs:8

  • The class name DashboardModel doesn't match the file name LogStatisticModel.cs. Consider renaming the file to DashboardModel.cs for consistency.
public class DashboardModel

tests/Serilog.Ui.PostgreSqlProvider.Tests/DataProvider/DataProviderDashboardTests.cs:157

  • The test method It_throws_when_skip_is_zero() throws NotImplementedException, indicating missing test coverage for this scenario in the dashboard tests.
    public override Task It_throws_when_skip_is_zero() => throw new NotImplementedException();

tests/Serilog.Ui.PostgreSqlProvider.Tests/DataProvider/DataProviderDashboardTests.cs:93

  • This test doesn't actually test an empty database scenario as noted in the comment (line 95-96). The test should either be implemented properly with an empty database or removed if not feasible.
    public async Task It_handles_empty_database_gracefully()

src/Serilog.Ui.PostgreSqlProvider/PostgresDataProvider.cs:61

  • Converting integer level to string and then back to level name through LogLevelConverter.GetLevelName(x.Level.ToString()) is inefficient and potentially error-prone. Consider using the integer level directly or create a method that takes an integer parameter.
            levelCounts.ToDictionary(x => LogLevelConverter.GetLevelName(x.Level.ToString()), x => x.Count);

Comment on lines +150 to +153
.Count(log => log.Timestamp.Date == today);

// Note: This might not be exact due to time zone handling and precise time boundaries
// but should be in the same ballpark

Copilot AI Aug 1, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The manual today count calculation uses .Date == today while the actual implementation likely uses range comparisons (>= today && < tomorrow). This discrepancy could cause test failures due to different date boundary logic.

Suggested change
.Count(log => log.Timestamp.Date == today);
// Note: This might not be exact due to time zone handling and precise time boundaries
// but should be in the same ballpark
.Count(log => log.Timestamp >= today && log.Timestamp < tomorrow);
// Note: This should align with the actual implementation's date boundary logic

Copilot uses AI. Check for mistakes.
DateTime today = DateTime.Today;
DateTime tomorrow = today.AddDays(1);

await using NpgsqlConnection connection = new(options.ConnectionString);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe these 4 requests can be awaited in parallel, using Task.WhenAll(), instead of doing them one by one 👍

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e069320. Refactored FetchDashboardAsync to run all 4 queries in parallel using Task.WhenAll(). Each query now uses its own NpgsqlConnection (since a single Npgsql connection can't handle concurrent queries), extracted into private ExecuteScalarAsync<TResult> and ExecuteQueryAsync<TResult> helpers.

@NandanDevHub NandanDevHub left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mo-esmp Two things i wanted to say, was going through this dashboard log query commits

The aggregation uses .Date when bucketing by day/week. If PostgreSQL is running in UTC but the app/UI expects local time, this risks off-by-one errors. normalizing to UTC explicitly at query time, or documenting that all dashboard ranges are UTC.

and The new PostgreSQL query selects timestamp and level without explicit composite index hints. For typical dashboard queries (time range + level), a (Timestamp, Level) index would help avoid sequential scans.

@mo-esmp

mo-esmp commented May 12, 2026

Copy link
Copy Markdown
Member Author

@copilot resolve the merge conflicts in this pull request

Copilot AI and others added 2 commits May 12, 2026 19:32
Co-authored-by: mo-esmp <1659032+mo-esmp@users.noreply.github.com>

Copilot AI commented May 12, 2026

Copy link
Copy Markdown

@copilot resolve the merge conflicts in this pull request

Resolved in d90bb61. Merged master into the feature branch, resolving conflicts in build/Build.Backend.Tests.cs, src/Serilog.Ui.PostgreSqlProvider/Models/PostgreLogModel.cs, and src/Serilog.Ui.SqliteDataProvider/SqliteDataProvider.cs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants