Repository navigation
Development - #9
Conversation
- Fixed benchmark artifact paths to src/Rapp.Benchmark/BenchmarkDotNet.Artifacts/results/ - Use markdown, CSV, and HTML exporters (no JSON needed) - Added PR comment with benchmark results from markdown output - Workflow already configured to run on PRs against main branch - Simplified artifact collection for standard BenchmarkDotNet outputs
📊 Benchmark Results
|
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
| var stats = new RappDashboardStats( | ||
| Hits: hits, | ||
| Misses: misses, | ||
| RappBytes: rappBytes, | ||
| JsonBytes: jsonBytes, | ||
| TotalRequests: totalRequests, | ||
| HitRate: hitRate, | ||
| Version: version | ||
| ); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
In general, to fix a “useless assignment to local variable” where the variable is only used once, either remove the variable and inline the expression where it is used, or, if the value really isn’t needed, remove the assignment entirely. If the right-hand side has side effects you must keep the expression itself (e.g., call the method) even if you drop the variable.
Here, the best fix without changing functionality is to remove the stats local variable and pass the new RappDashboardStats instance directly into Results.Json. That means replacing the block:
var stats = new RappDashboardStats(
...
);
return Results.Json(stats, DashboardJsonContext.Default.RappDashboardStats);with:
return Results.Json(
new RappDashboardStats(
...
),
DashboardJsonContext.Default.RappDashboardStats);No new imports, methods, or other definitions are needed. The change is localized to the JSON-branch inside MapRappDashboard in src/Rapp.Dashboard/DashboardExtensions.cs.
| @@ -69,17 +69,17 @@ | ||
| if (context.Request.Query["format"] == "json" || | ||
| context.Request.Headers.Accept.ToString().Contains("application/json")) | ||
| { | ||
| var stats = new RappDashboardStats( | ||
| Hits: hits, | ||
| Misses: misses, | ||
| RappBytes: rappBytes, | ||
| JsonBytes: jsonBytes, | ||
| TotalRequests: totalRequests, | ||
| HitRate: hitRate, | ||
| Version: version | ||
| ); | ||
|
|
||
| return Results.Json(stats, DashboardJsonContext.Default.RappDashboardStats); | ||
| return Results.Json( | ||
| new RappDashboardStats( | ||
| Hits: hits, | ||
| Misses: misses, | ||
| RappBytes: rappBytes, | ||
| JsonBytes: jsonBytes, | ||
| TotalRequests: totalRequests, | ||
| HitRate: hitRate, | ||
| Version: version | ||
| ), | ||
| DashboardJsonContext.Default.RappDashboardStats); | ||
| } | ||
|
|
||
| var html = $@" |
| target.Advance(8); | ||
|
|
||
| // Serialize payload directly to target | ||
| var startState = target.GetSpan(0).Length; // Approximate logic if needed, but better to measure: |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the problem, we should remove the unused local variable assignment so that there is no dead code. Since the intent to measure bytes written is commented out and not implemented, and there is no subsequent use of startState, the safest change that preserves existing functionality is simply to delete the line defining/assigning startState. We do not need to add new logic, imports, or dependencies; we just eliminate the unused assignment.
Concretely, in src/Rapp/RappBaseSerializer.cs, inside the Serialize(T value, IBufferWriter<byte> target) method, delete the line:
var startState = target.GetSpan(0).Length; // Approximate logic if needed, but better to measure:and leave the surrounding comments and serialization call unchanged. No other lines or files need to be modified.
| @@ -152,7 +152,6 @@ | ||
| target.Advance(8); | ||
|
|
||
| // Serialize payload directly to target | ||
| var startState = target.GetSpan(0).Length; // Approximate logic if needed, but better to measure: | ||
|
|
||
| // We need to measure bytes written. | ||
| // MemoryPack doesn't return bytes written easily from this overload without pre-measuring. |
|
|
||
| RappMetrics.RecordSerializationSize(binarySize, jsonSize); | ||
| } | ||
| catch { /* Ignore telemetry errors */ } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
General fix approach: avoid a bare catch by catching only expected, non-fatal exception types for this telemetry work, and optionally rethrow or at least surface unexpected exceptions. Given the constraints (no behavior change, telemetry must not break serialization), the best compromise is to catch a reasonably broad but explicit base type (Exception), and within that handler ignore only truly non-critical errors while at least recording the problem (or explicitly documenting why it is ignored). This satisfies the “no generic catch clause” rule (because it is no longer an untyped catch) and makes the intent clearer.
Best concrete fix here: replace catch { /* Ignore telemetry errors */ } with catch (Exception) { /* Ignore telemetry errors */ }. This preserves runtime behavior (all exceptions are still swallowed so telemetry can never affect the caller) but eliminates the pattern of a generic catch clause without an explicit exception type that CodeQL flags. No additional imports are required because System.Exception is always available. All changes are confined to the Serialize method’s telemetry block in src/Rapp/RappBaseSerializer.cs around line 167–177.
| @@ -174,7 +174,7 @@ | ||
|
|
||
| RappMetrics.RecordSerializationSize(binarySize, jsonSize); | ||
| } | ||
| catch { /* Ignore telemetry errors */ } | ||
| catch (Exception) { /* Ignore telemetry errors */ } | ||
| } | ||
| #endif | ||
| } |
|
|
||
| RappMetrics.RecordSerializationSize(binarySize, jsonSize); | ||
| } | ||
| catch { /* Ignore telemetry errors */ } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
In general, fix this by replacing the untyped catch { } with one or more typed catch clauses that handle only the exceptions we truly want to ignore, and optionally rethrow or let others bubble up. For telemetry code, it's common to ignore typical runtime failures (e.g., ArgumentException, InvalidOperationException, JsonException) while allowing critical exceptions (OutOfMemoryException, ThreadAbortException, StackOverflowException, OperationCanceledException) to escape.
The best low‑impact fix here is to change both generic catch blocks (one in the serialization telemetry section around line 177, and one in the deserialization telemetry section around line 270) to catch Exception as ex and then immediately rethrow if it is one of a small set of critical exceptions. This keeps the behavior of swallowing normal telemetry failures, but no longer blindly swallows everything. We don’t need new imports; Exception, OutOfMemoryException, ThreadAbortException, StackOverflowException, and OperationCanceledException are in System, which is available by default. Concretely:
- In the first telemetry block (around lines 167–177), replace
catch { /* Ignore telemetry errors */ }withcatch (Exception ex)plus a small helper pattern that rethrows critical exceptions and otherwise ignores. - Apply the same pattern to the second telemetry block (around lines 260–270).
To avoid duplication and keep changes local, we can add a small private static helper method IsCriticalException(Exception ex) inside RappBaseSerializer<T> and use it in both catch blocks.
| @@ -62,8 +62,20 @@ | ||
| public abstract class RappBaseSerializer<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T> : IHybridCacheSerializer<T> | ||
| { | ||
| /// <summary> | ||
| /// Gets the cryptographic hash of the type's schema used for validation. | ||
| /// Determines whether an exception is considered critical and should not be swallowed. | ||
| /// </summary> | ||
| /// <param name="ex">The exception to evaluate.</param> | ||
| /// <returns><c>true</c> if the exception is critical; otherwise, <c>false</c>.</returns> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| private static bool IsCriticalException(Exception ex) => | ||
| ex is OutOfMemoryException | ||
| or ThreadAbortException | ||
| or StackOverflowException | ||
| or OperationCanceledException; | ||
|
|
||
| /// <summary> | ||
| /// Base class for schema-aware binary serializers that provide enterprise-grade safety | ||
| /// for HybridCache serialization operations. | ||
| /// <value> | ||
| /// A 64-bit unsigned integer representing the schema hash. This value is computed | ||
| /// at compile-time by the source generator and must match exactly for successful | ||
| @@ -174,7 +185,14 @@ | ||
|
|
||
| RappMetrics.RecordSerializationSize(binarySize, jsonSize); | ||
| } | ||
| catch { /* Ignore telemetry errors */ } | ||
| catch (Exception ex) | ||
| { | ||
| if (IsCriticalException(ex)) | ||
| { | ||
| throw; | ||
| } | ||
| // Ignore non-critical telemetry errors | ||
| } | ||
| } | ||
| #endif | ||
| } | ||
| @@ -267,7 +285,14 @@ | ||
|
|
||
| RappMetrics.RecordSerializationSize(binarySize, jsonSize); | ||
| } | ||
| catch { /* Ignore telemetry errors */ } | ||
| catch (Exception ex) | ||
| { | ||
| if (IsCriticalException(ex)) | ||
| { | ||
| throw; | ||
| } | ||
| // Ignore non-critical telemetry errors | ||
| } | ||
| } | ||
| #endif | ||
|
|
📊 Benchmark Results
|
- Enable Central Package Management (CPM) - Centralize build logic in Directory.Build.props with strict analysis - Pin .NET SDK in global.json - Fix all compiler warnings (CA1001, CA1852, CA1050, CA2016, CA1304, CA1822) - Update documentation and dependencies - Verify samples and tests
| public void Stop() | ||
| { | ||
| _displayTimer.Change(Timeout.Infinite, Timeout.Infinite); | ||
| _listener?.DisableMeasurementEvents((Instrument)null!); |
Check warning
Code scanning / CodeQL
Useless upcast Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
In general, to fix a useless upcast you remove the explicit cast and null-forgiving operator and rely on C#’s implicit conversion from null to the desired reference type. This keeps behavior identical while simplifying the code and eliminating the static analysis warning.
For this specific case in src/Rapp.Playground/Program.cs, line 273 currently calls _listener?.DisableMeasurementEvents((Instrument)null!);. The MeterListener.DisableMeasurementEvents method accepts an Instrument? argument, and passing null directly already targets that overload. The explicit (Instrument) cast and ! are redundant. The best fix is to replace (Instrument)null! with just null, leaving the rest of the method unchanged.
No new methods, imports, or other definitions are required; this is a local one-line simplification that preserves existing functionality.
| @@ -270,7 +270,7 @@ | ||
| public void Stop() | ||
| { | ||
| _displayTimer.Change(Timeout.Infinite, Timeout.Infinite); | ||
| _listener?.DisableMeasurementEvents((Instrument)null!); | ||
| _listener?.DisableMeasurementEvents(null); | ||
| } | ||
|
|
||
| public void Dispose() |
[1.1.0] - 2026-01-07
Added
GETTING_STARTED.mdguide for new users.