feat: Add errors, health and app metrics in Gateway API #17299
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/PodsClient.cs (4)
27-33: Critical: Null check occurs after property access, defeating its purpose.Line 27 directly accesses
deployment.Spec.Selector.MatchLabelsbefore the null check on line 28. IfSpec,Selector, orMatchLabelsis null, aNullReferenceExceptionwill be thrown on line 27 before the guard clause is evaluated.🔎 Proposed fix: move null check before assignment
- var selector = deployment.Spec.Selector.MatchLabels; - if (selector == null || selector.Count == 0) + if (deployment.Spec?.Selector?.MatchLabels == null || deployment.Spec.Selector.MatchLabels.Count == 0) { throw new InvalidOperationException( $"Deployment {org}-{app}-deployment-v2 has no label selector configured." ); } + + var selector = deployment.Spec.Selector.MatchLabels;
12-12: Hardcoded namespace reduces flexibility.The namespace is hardcoded to "default". This should be configurable via settings or derived from context to support different deployment configurations.
🔎 Proposed configuration approach
Consider adding a namespace property to
GatewayContextor creating a Kubernetes-specific settings class:+ // In GatewayContext or new KubernetesSettings class + public string KubernetesNamespace { get; set; } = "default"; public async Task<double> GetReadyPodsCountAsync(string app, CancellationToken cancellationToken) { string org = gatewayContext.ServiceOwner; - var namespaceName = "default"; + var namespaceName = gatewayContext.KubernetesNamespace;
14-18: Hardcoded deployment naming pattern and missing error handling.The deployment name pattern
{org}-{app}-deployment-v2is hardcoded with a "-v2" suffix, which appears arbitrary and may break if naming conventions change. Additionally, there's no error handling if the deployment doesn't exist, which will cause an unhandled exception.🔎 Proposed improvements
+ string deploymentName = $"{org}-{app}-deployment-v2"; + - var deployment = await client.AppsV1.ReadNamespacedDeploymentAsync( - $"{org}-{app}-deployment-v2", - namespaceName, - cancellationToken: cancellationToken - ); + V1Deployment deployment; + try + { + deployment = await client.AppsV1.ReadNamespacedDeploymentAsync( + deploymentName, + namespaceName, + cancellationToken: cancellationToken + ); + } + catch (k8s.Autorest.HttpOperationException ex) when (ex.Response.StatusCode == System.Net.HttpStatusCode.NotFound) + { + // Return 0 or throw a more specific exception + return 0; + }Consider making the deployment naming pattern configurable or at least document why "-v2" is required.
22-25: Consider edge case when desiredReplicas is explicitly zero.Whilst the null-coalescing operator on line 20 provides a fallback to 0, if
deployment.Spec.Replicasis explicitly set to 0 (e.g., scaled down deployment), the calculation returns 100. Verify whether returning 100 is appropriate for scaled-down deployments, or if 0 would be more accurate.
🧹 Nitpick comments (4)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs (4)
22-22: Consider usingIReadOnlyDictionaryfor immutability.Since this dictionary is never modified after initialisation, using
IReadOnlyDictionary<string, string[]>would better express the intent and prevent accidental modifications.🔎 Proposed fix
- private static readonly IDictionary<string, string[]> _operationNames = new Dictionary<string, string[]> + private static readonly IReadOnlyDictionary<string, string[]> _operationNames = new Dictionary<string, string[]>
58-59: Missing cancellation token propagation to ARM API calls.The
GetAsynccalls do not pass a cancellation token, meaning callers cannot cancel long-running ARM requests. Consider adding aCancellationTokenparameter to this method and propagating it.🔎 Proposed fix
- private async Task<string> GetApplicationLogAnalyticsWorkspaceIdAsync() + private async Task<string> GetApplicationLogAnalyticsWorkspaceIdAsync(CancellationToken cancellationToken = default) { if (_workspaceId != null) return _workspaceId; string resourceGroupName = $"monitor-{gatewayContext.ServiceOwner}-{gatewayContext.Environment}-rg"; string workspaceName = $"application-{gatewayContext.ServiceOwner}-{gatewayContext.Environment}-law"; var subscription = armClient.GetSubscriptionResource( SubscriptionResource.CreateResourceIdentifier(gatewayContext.AzureSubscriptionId) ); - var rg = await subscription.GetResourceGroups().GetAsync(resourceGroupName); - var workspace = await rg.Value.GetOperationalInsightsWorkspaces().GetAsync(workspaceName); + var rg = await subscription.GetResourceGroups().GetAsync(resourceGroupName, cancellationToken); + var workspace = await rg.Value.GetOperationalInsightsWorkspaces().GetAsync(workspaceName, cancellationToken);Then update all call sites (lines 74, 123, 180) to pass the cancellation token.
184-184: Consider extracting hardcoded metric names to a class-level constant.Similar to
_operationNames, extracting these metric names would improve discoverability and make it easier to add new metrics in the future.🔎 Proposed fix
Add near the top of the class:
private static readonly string[] _appMetricNames = [ "altinn_app_lib_processes_started", "altinn_app_lib_processes_completed" ];Then update line 184:
- List<string> names = ["altinn_app_lib_processes_started", "altinn_app_lib_processes_completed"]; + var names = _appMetricNames;
236-237: Consider caching the template file content.The JSON template is read from disk on every call to
GetLogsUrl. Since the file content is static, caching it in astatic readonlyfield (using lazy initialisation) would eliminate repeated file I/O.🔎 Proposed fix
+ private static readonly Lazy<string> _logsQueryTemplate = new(() => + { + string jsonPath = Path.Combine(AppContext.BaseDirectory, "Clients", "MetricsClient", "logsQueryTemplate.json"); + return File.ReadAllText(jsonPath); + }); + public Uri GetLogsUrl(string subscriptionId, string org, string env, string appName, string metricName, int range) { ArgumentOutOfRangeException.ThrowIfNegativeOrZero(range); ArgumentOutOfRangeException.ThrowIfGreaterThan(range, MaxRange); if (!_operationNames.TryGetValue(metricName, out var operationNames)) { throw new ArgumentException($"Unknown metric name: {metricName}", nameof(metricName)); } - string jsonPath = Path.Combine(AppContext.BaseDirectory, "Clients", "MetricsClient", "logsQueryTemplate.json"); - string jsonTemplate = File.ReadAllText(jsonPath); + string jsonTemplate = _logsQueryTemplate.Value; string json = jsonTemplate
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Runtime/StudioGateway/src/StudioGateway.Api/Application/HandleMetrics.cs(1 hunks)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/GrafanaClient.cs(1 hunks)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/PodsClient.cs(1 hunks)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs(1 hunks)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/logsQueryTemplate.json(1 hunks)src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
📚 Learning: 2025-12-11T09:10:06.960Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 17174
File: src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs:1-8
Timestamp: 2025-12-11T09:10:06.960Z
Learning: In C# 12, you can declare a class, interface, or struct with an empty body using a trailing semicolon as a shorthand (e.g., internal sealed class X : Y;). When reviewing StudioGateway.Api code under src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs and other C# files, verify the language version is set to at least 12.0+ and that any empty-body types use the single semicolon shorthand only when semantically appropriate (i.e., the type has no members). If a type previously had members, continue using a normal body.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/GrafanaClient.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/PodsClient.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Application/HandleMetrics.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
📚 Learning: 2025-01-28T22:18:03.328Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:43-48
Timestamp: 2025-01-28T22:18:03.328Z
Learning: Error handling improvements for the Get method in AzureDevOpsBuildClient were deemed out of scope for PR #14456, which focused on undeploy endpoints functionality.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/PodsClient.cs
📚 Learning: 2025-01-28T14:32:12.914Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/Repository/ORMImplementation/DeploymentRepository.cs:50-58
Timestamp: 2025-01-28T14:32:12.914Z
Learning: In the Altinn Studio project, methods retrieving deployment entities should fail fast (throw exceptions) when no matching records are found, rather than handling null cases. This is demonstrated in the `GetLastDeployed` method of `DeploymentRepository` which uses `FirstAsync()` instead of `FirstOrDefaultAsync()`.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/PodsClient.cs
📚 Learning: 2025-11-25T08:20:59.697Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/*.cs : Do not use Async suffix for async methods
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Application/HandleMetrics.cs
📚 Learning: 2025-11-25T08:20:59.697Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/*.cs : Do not use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Application/HandleMetrics.cs
📚 Learning: 2025-11-25T08:20:59.697Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/*.cs : Do not await async operations in a loop - prefer batching with an upper bound on parallelism
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Application/HandleMetrics.cs
📚 Learning: 2025-11-25T08:20:59.697Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/*.cs : Do not invoke the same async operation multiple times in the same codepath unless necessary
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Application/HandleMetrics.cs
📚 Learning: 2025-11-25T08:20:59.697Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/*.cs : Register services in DI container properly following existing patterns for options configuration
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
🧬 Code graph analysis (3)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/GrafanaClient.cs (2)
src/Runtime/StudioGateway/src/StudioGateway.Api/Application/HandleAlerts.cs (2)
Task(11-39)Task(41-49)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/IAlertsClient.cs (1)
Task(13-13)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/PodsClient.cs (3)
src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/GatewayContext.cs (1)
GatewayContext(3-9)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs (4)
Task(47-67)Task(69-112)Task(114-169)Task(171-224)src/Runtime/operator/internal/operatorcontext/operatorcontext.go (1)
ServiceOwner(32-36)
src/Runtime/StudioGateway/src/StudioGateway.Api/Application/HandleMetrics.cs (9)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs (1)
Uri(226-255)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/IMetricsClient.cs (1)
Uri(20-20)src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/GatewayContext.cs (1)
GatewayContext(3-9)src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/MetricsClientSettings.cs (1)
MetricsClientSettings(3-6)src/Runtime/StudioGateway/src/StudioGateway.Contracts/Metrics/ErrorMetric.cs (1)
ErrorMetric(3-8)src/Runtime/StudioGateway/src/StudioGateway.Contracts/Metrics/AppMetric.cs (1)
AppMetric(3-7)src/Runtime/StudioGateway/src/StudioGateway.Contracts/Metrics/AppMetricDataPoint.cs (1)
AppMetricDataPoint(3-7)src/Runtime/StudioGateway/src/StudioGateway.Contracts/Metrics/AppErrorMetric.cs (1)
AppErrorMetric(3-7)src/Runtime/StudioGateway/src/StudioGateway.Contracts/Metrics/AppHealthMetric.cs (1)
AppHealthMetric(3-7)
🪛 Biome (2.1.2)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/logsQueryTemplate.json
[error] 66-66: Property key must be double quoted
(parse)
[error] 66-66: expected : but instead found }
Remove }
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test
🔇 Additional comments (13)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs (1)
114-169: LGTM!Good implementation with proper input escaping for the
appparameter (line 133) and consistent validation. The pattern of returning all metric names with empty data points for missing entries provides a predictable response structure.src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/GrafanaClient.cs (1)
5-16: LGTM! Clean implementation of Grafana alert rules retrieval.The implementation correctly uses source-generated JSON serialization via
AppJsonSerializerContextand provides a sensible empty collection fallback. The HTTP client error handling follows standard patterns whereGetFromJsonAsyncwill throwHttpRequestExceptionfor non-success status codes, which is appropriate for this scenario.src/Runtime/StudioGateway/src/StudioGateway.Api/Application/HandleMetrics.cs (5)
10-31: LGTM! Proper async pattern with keyed service resolution.The method correctly resolves
IMetricsClientvia keyed DI, awaits the async operation, and maps results to the contract DTO. Error handling will be managed by the metrics client implementation and ASP.NET Core middleware.
33-58: LGTM! Proper async pattern with keyed service resolution.The method correctly resolves
IMetricsClientvia keyed DI, awaits the async operation, and maps results to the contract DTOs with nested data points. The Select operations are appropriately deferred.
60-85: LGTM! Proper async pattern with keyed service resolution.The method correctly resolves
IMetricsClientvia keyed DI, awaits the async operation for app-specific failed requests, and maps results with nested data points. Implementation follows established patterns.
87-110: LGTM! Correctly implemented as synchronous method.The method appropriately returns
IResult(notTask<IResult>) sinceGetLogsUrlis synchronous. This avoids unnecessary async state machine overhead. The implementation correctly resolves the keyed service and constructs the Azure portal URL.
112-126: LGTM! Proper async pattern for health metrics.The method correctly uses
PodsClientto retrieve ready pods count and returns a structured health metric. Implementation is clean and follows async best practices.src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs (6)
75-76: LGTM! Required DI registrations now present.The registrations for
IHttpContextAccessorandGrafanaAuthenticationHandlercorrectly address the dependency requirements for the HTTP message handler pipeline. This resolves the critical issue identified in previous reviews.
23-33: LGTM! Proper Azure credential and client registrations.The singleton registrations for
TokenCredential,LogsQueryClient, andArmClientfollow best practices. UsingDefaultAzureCredentialprovides flexible authentication across environments, and the clients are correctly instantiated with the shared credential.
63-66: LGTM! Kubernetes client properly registered.The singleton registration uses
BuildDefaultConfig()which will correctly resolve configuration from the in-cluster service account or kubeconfig. Singleton lifetime is appropriate for the Kubernetes client.
67-74: LGTM! Keyed service registration follows DI patterns.The keyed transient registration for
IAlertsClientwith key "grafana" correctly resolves the named HttpClient and constructsGrafanaClient. The factory pattern ensures proper client lifecycle management.
88-88: LGTM! Keyed metrics client registration.The keyed transient registration for
IMetricsClientwith key "azuremonitor" follows the same pattern as the alerts client, enabling provider-based resolution in handlers.
77-87: The implementation already supports dynamic BaseAddress updates—no issue here.The factory delegate passed to
AddHttpClientis executed each timeCreateClient("grafana")is called, not once at startup. SinceGrafanaClientis registered asTransient, every request creates a newGrafanaClientinstance, which triggers a fresh factory call. Each factory call reads the currentGrafanaSettingsfromIOptionsSnapshotand creates a newHttpClientwith the updatedBaseAddress. Therefore, theBaseAddressdoes reflect runtime configuration changes.The suggestion to use a
DelegatingHandlerfor dynamic URL resolution is unnecessary given the current architecture. Adding URL validation (checking for null/empty) remains a reasonable defensive measure.
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/logsQueryTemplate.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs (4)
22-40: Consider usingIReadOnlyDictionaryfor immutable dictionary.The dictionary is effectively immutable but declared as
IDictionary, which signals mutability. UsingIReadOnlyDictionary(orFrozenDictionaryfor .NET 8+) would better express the intent and prevent accidental modification.🔎 Proposed fix
- private static readonly IDictionary<string, string[]> _operationNames = new Dictionary<string, string[]> + private static readonly IReadOnlyDictionary<string, string[]> _operationNames = new Dictionary<string, string[]>
47-67: Lazy caching is not thread-safe.Multiple concurrent calls could all pass the null check on line 49, causing redundant ARM API calls. Consider using
Lazy<Task<string>>or a lock for thread-safe initialisation.🔎 Proposed fix using Lazy
Add a field and update the method:
- private string? _workspaceId; + private readonly Lazy<Task<string>> _workspaceIdLazy; + // In constructor or field initializer: + public AzureMonitorClient(...) + { + _workspaceIdLazy = new Lazy<Task<string>>(FetchWorkspaceIdAsync); + } - private async Task<string> GetApplicationLogAnalyticsWorkspaceIdAsync() + private Task<string> GetApplicationLogAnalyticsWorkspaceIdAsync() => _workspaceIdLazy.Value; + + private async Task<string> FetchWorkspaceIdAsync() { - if (_workspaceId != null) - return _workspaceId; - string resourceGroupName = $"monitor-{gatewayContext.ServiceOwner}-{gatewayContext.Environment}-rg"; string workspaceName = $"application-{gatewayContext.ServiceOwner}-{gatewayContext.Environment}-law"; var subscription = armClient.GetSubscriptionResource( SubscriptionResource.CreateResourceIdentifier(gatewayContext.AzureSubscriptionId) ); var rg = await subscription.GetResourceGroups().GetAsync(resourceGroupName); var workspace = await rg.Value.GetOperationalInsightsWorkspaces().GetAsync(workspaceName); - _workspaceId = - workspace.Value.Data.CustomerId?.ToString() ?? throw new InvalidOperationException( - "Log Analytics Workspace ID not found." - ); - - return _workspaceId; + return workspace.Value.Data.CustomerId?.ToString() + ?? throw new InvalidOperationException("Log Analytics Workspace ID not found."); }
184-184: Consider extracting metric names to a constant.For consistency with
_operationNamesand easier maintenance, the hardcoded list could be extracted to a class-level constant.🔎 Proposed refactor
+ private static readonly string[] _appMetricNames = + [ + "altinn_app_lib_processes_started", + "altinn_app_lib_processes_completed" + ]; + // In GetAppMetricsAsync: - List<string> names = ["altinn_app_lib_processes_started", "altinn_app_lib_processes_completed"]; + var names = _appMetricNames;
236-237: Synchronous file I/O on every call is inefficient.
File.ReadAllTextis called synchronously on every invocation ofGetLogsUrl. Since the template content is static, consider caching it at startup or usingLazy<string>to read it once.🔎 Proposed fix with lazy caching
+ private static readonly Lazy<string> _logsQueryTemplate = new(() => + { + string jsonPath = Path.Combine(AppContext.BaseDirectory, "Clients", "MetricsClient", "logsQueryTemplate.json"); + return File.ReadAllText(jsonPath); + }); + public Uri GetLogsUrl(string subscriptionId, string org, string env, string appName, string metricName, int range) { // ... validation ... - string jsonPath = Path.Combine(AppContext.BaseDirectory, "Clients", "MetricsClient", "logsQueryTemplate.json"); - string jsonTemplate = File.ReadAllText(jsonPath); + string jsonTemplate = _logsQueryTemplate.Value; string json = jsonTemplate
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Runtime/StudioGateway/infra/kustomize/local-syncroot/kustomization.yaml(1 hunks)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs(1 hunks)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/logsQueryTemplate.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/logsQueryTemplate.json
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
📚 Learning: 2025-12-11T09:10:06.960Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 17174
File: src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs:1-8
Timestamp: 2025-12-11T09:10:06.960Z
Learning: In C# 12, you can declare a class, interface, or struct with an empty body using a trailing semicolon as a shorthand (e.g., internal sealed class X : Y;). When reviewing StudioGateway.Api code under src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs and other C# files, verify the language version is set to at least 12.0+ and that any empty-body types use the single semicolon shorthand only when semantically appropriate (i.e., the type has no members). If a type previously had members, continue using a normal body.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs
📚 Learning: 2025-12-17T11:31:17.964Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/Runtime/operator/CLAUDE.md:0-0
Timestamp: 2025-12-17T11:31:17.964Z
Learning: Applies to src/Runtime/operator/config/**/* : Place Kubernetes manifests and Kustomize configurations in `config/` directory
Applied to files:
src/Runtime/StudioGateway/infra/kustomize/local-syncroot/kustomization.yaml
🧬 Code graph analysis (1)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs (4)
src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/GatewayContext.cs (1)
GatewayContext(3-9)src/Runtime/StudioGateway/src/StudioGateway.Api/Application/HandleMetrics.cs (4)
Task(10-31)Task(33-58)Task(60-85)Task(112-126)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/IMetricsClient.cs (4)
Task(13-13)Task(14-18)Task(19-19)Uri(20-20)src/Runtime/StudioGateway/src/StudioGateway.Contracts/Metrics/AppMetric.cs (1)
AppMetric(3-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test
🔇 Additional comments (2)
src/Runtime/StudioGateway/infra/kustomize/local-syncroot/kustomization.yaml (1)
5-5: Verify that flux-config.yaml contains necessary Flux resources and Alert patches remain valid.The replacement of
flux-notifications.yamlwithflux-config.yamlas a base resource requires confirmation that:
flux-config.yamlexists and is properly referenced at the relative path- Any Alert patch targeting notification.toolkit.fluxcd.io/v1beta3 will continue to work (Alert resources are standard Flux CRDs and do not depend on a specific base configuration file containing them)
- If
flux-notifications.yamlis referenced elsewhere in the codebase, this change may cause breakagePlease confirm the files exist and that no other kustomizations depend on
flux-notifications.yaml.src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs (1)
114-169: LGTM!The implementation correctly escapes the
appparameter for KQL injection prevention and ensures all expected metric names are returned (with emptyDataPointsfor any not found in results). The same.First()concern fromGetFailedRequestsAsyncapplies to line 150, but the fix would be analogous.
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cs(2 hunks)src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
📚 Learning: 2025-01-28T22:20:43.402Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:70-75
Timestamp: 2025-01-28T22:20:43.402Z
Learning: The QueueBuildRequest class in Altinn Studio uses JsonPropertyName attributes from System.Text.Json for serialization (e.g., [JsonPropertyName("definition")] for the DefinitionReference property).
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cs
📚 Learning: 2025-04-29T08:18:57.422Z
Learnt from: mgunnerud
Repo: Altinn/altinn-studio PR: 15272
File: backend/src/Designer/Models/ConsentMetadata.cs:0-0
Timestamp: 2025-04-29T08:18:57.422Z
Learning: In the Altinn Studio codebase, C# model classes use PascalCase for property names without explicit JsonPropertyName attributes. The application likely uses a global JSON serialization configuration that automatically handles case conversion between C# PascalCase and JavaScript camelCase.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cs
📚 Learning: 2025-04-29T08:18:57.422Z
Learnt from: mgunnerud
Repo: Altinn/altinn-studio PR: 15272
File: backend/src/Designer/Models/ConsentMetadata.cs:0-0
Timestamp: 2025-04-29T08:18:57.422Z
Learning: In the Altinn Studio codebase, C# model classes use PascalCase for property names without explicit JsonPropertyName attributes. The ASP.NET Core framework automatically handles serialization between C# PascalCase and JSON camelCase without requiring explicit property name mapping.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cs
📚 Learning: 2025-11-25T08:20:59.697Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/test/**/*.cs : Use snapshot testing for OpenAPI docs and telemetry output
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cs
📚 Learning: 2025-01-28T22:20:43.402Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:70-75
Timestamp: 2025-01-28T22:20:43.402Z
Learning: The QueueBuildRequest class in Altinn Studio uses System.Text.Json serialization annotations directly on the class properties, making explicit serializer options unnecessary.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cs
📚 Learning: 2025-12-11T09:10:06.960Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 17174
File: src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs:1-8
Timestamp: 2025-12-11T09:10:06.960Z
Learning: In C# 12, you can declare a class, interface, or struct with an empty body using a trailing semicolon as a shorthand (e.g., internal sealed class X : Y;). When reviewing StudioGateway.Api code under src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs and other C# files, verify the language version is set to at least 12.0+ and that any empty-body types use the single semicolon shorthand only when semantically appropriate (i.e., the type has no members). If a type previously had members, continue using a normal body.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
📚 Learning: 2025-11-25T08:20:59.697Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/*.cs : Register services in DI container properly following existing patterns for options configuration
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
🧬 Code graph analysis (2)
src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cs (5)
src/Runtime/StudioGateway/src/StudioGateway.Contracts/Alerts/AlertRule.cs (1)
AlertRule(5-36)src/Runtime/StudioGateway/src/StudioGateway.Contracts/Metrics/ErrorMetric.cs (1)
ErrorMetric(3-8)src/Runtime/StudioGateway/src/StudioGateway.Contracts/Metrics/AppMetric.cs (1)
AppMetric(3-7)src/Runtime/StudioGateway/src/StudioGateway.Contracts/Metrics/AppErrorMetric.cs (1)
AppErrorMetric(3-7)src/Runtime/StudioGateway/src/StudioGateway.Contracts/Metrics/AppHealthMetric.cs (1)
AppHealthMetric(3-7)
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs (7)
src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/GrafanaSettings.cs (1)
GrafanaSettings(3-7)src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/AlertsClientSettings.cs (1)
AlertsClientSettings(3-6)src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/MetricsClientSettings.cs (1)
MetricsClientSettings(3-6)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/GrafanaClient.cs (1)
GrafanaClient(5-17)src/Runtime/StudioGateway/src/StudioGateway.Api/Authentication/GrafanaAuthenticationHandler.cs (2)
GrafanaAuthenticationHandler(6-29)GrafanaAuthenticationHandler(10-13)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs (2)
Uri(226-255)AzureMonitorClient(12-256)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/IMetricsClient.cs (1)
Uri(20-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test
🔇 Additional comments (8)
src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cs (1)
2-2: LGTM!The using directives correctly reference the namespaces for the new types being added to the serializer context.
Also applies to: 7-7, 9-9
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs (7)
23-33: LGTM! Azure client registrations follow best practices.The Azure client registrations are well-structured:
DefaultAzureCredentialfor flexible authentication across environments- Singleton lifetime appropriate for Azure SDK clients
- Proper dependency injection pattern with
TokenCredentialas the foundation
40-44: Configuration setup supports dynamic reloading.The configuration bindings correctly enable runtime updates via
reloadOnChange: true, allowing secrets to be rotated without application restart.
48-51: Scoped registrations correctly support dynamic configuration.The use of
IOptionsSnapshotwith scoped lifetime allows configuration changes to be picked up per-request without requiring application restart. The explanatory comment adds valuable context.
63-66: Kubernetes client registration follows standard patterns.Using
BuildDefaultConfig()and singleton lifetime is the recommended approach for the Kubernetes .NET client.
67-74: Keyed service registration enables future extensibility.The keyed
IAlertsClientregistration with provider key "grafana" allows for multiple alert provider implementations whilst maintaining clean dependency injection.
75-76: Required DI registrations now present.The registrations for
IHttpContextAccessorandGrafanaAuthenticationHandlercorrectly address the previous review comment. These are necessary for the handler to resolveGrafanaSettingsper-request.
87-87: Metrics client registration follows consistent pattern.The keyed
IMetricsClientregistration mirrors the alerts client approach, allowing for multiple metrics provider implementations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/Contracts/AzureMonitor/DataPoint.cs (1)
5-5: Consider renaming the property to improve clarity.The property name
DateTimeOffsetmatches its type name, which can reduce readability. Consider renaming to something more descriptive such asTimestamp,ObservedAt, orTime.🔎 Suggested refactor
- public DateTimeOffset DateTimeOffset { get; set; } + public DateTimeOffset Timestamp { get; set; }src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/IAlertsClient.cs (1)
7-7: Optional: Remove redundantpublicmodifier.Interface members are implicitly public in C#, so the
publicmodifier is redundant here.🔎 Proposed fix
- public Task<IEnumerable<GrafanaAlertRule>> GetAlertRulesAsync(CancellationToken cancellationToken); + Task<IEnumerable<GrafanaAlertRule>> GetAlertRulesAsync(CancellationToken cancellationToken);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/Contracts/GrafanaAlertRule.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/IAlertsClient.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/Contracts/AzureMonitor/AppFailedRequest.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/Contracts/AzureMonitor/AppMetric.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/Contracts/AzureMonitor/DataPoint.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/Contracts/AzureMonitor/FailedRequest.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/IMetricsClient.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/IMetricsClient.cs
- src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/Contracts/AzureMonitor/FailedRequest.cs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
📚 Learning: 2025-12-11T09:10:06.960Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 17174
File: src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs:1-8
Timestamp: 2025-12-11T09:10:06.960Z
Learning: In C# 12, you can declare a class, interface, or struct with an empty body using a trailing semicolon as a shorthand (e.g., internal sealed class X : Y;). When reviewing StudioGateway.Api code under src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs and other C# files, verify the language version is set to at least 12.0+ and that any empty-body types use the single semicolon shorthand only when semantically appropriate (i.e., the type has no members). If a type previously had members, continue using a normal body.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/Contracts/AzureMonitor/AppFailedRequest.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/Contracts/GrafanaAlertRule.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/Contracts/AzureMonitor/AppMetric.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/IAlertsClient.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/Contracts/AzureMonitor/DataPoint.cs
🧬 Code graph analysis (3)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/Contracts/AzureMonitor/AppFailedRequest.cs (1)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/Contracts/AzureMonitor/DataPoint.cs (1)
DataPoint(3-7)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/Contracts/AzureMonitor/AppMetric.cs (1)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/Contracts/AzureMonitor/DataPoint.cs (1)
DataPoint(3-7)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/IAlertsClient.cs (3)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/GrafanaClient.cs (1)
Task(8-16)src/Runtime/StudioGateway/src/StudioGateway.Api/Application/HandleAlerts.cs (2)
Task(11-39)Task(41-49)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/Contracts/GrafanaAlertRule.cs (1)
GrafanaAlertRule(5-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test
🔇 Additional comments (4)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/Contracts/AzureMonitor/AppFailedRequest.cs (1)
3-7: LGTM!The data contract is well-structured. The use of
requiredproperties ensures non-null values at construction time, which is appropriate for this DTO. Note thatAppFailedRequestandAppMetricshare an identical structure—this appears intentional for type safety and domain separation, allowing distinct semantics for failed requests versus general metrics.src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/Contracts/AzureMonitor/AppMetric.cs (1)
3-7: LGTM!The data contract is well-structured and consistent with the other Azure Monitor contracts. The use of
requiredproperties ensures type safety whilst maintaining compatibility with JSON serialisation requirements.src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/Contracts/GrafanaAlertRule.cs (2)
5-63: LGTM!The contract classes are well-structured for JSON deserialization with appropriate use of
JsonPropertyNameattributes. The nestedGrafanaAnnotationsandGrafanaLabelsclasses provide a clean mapping to Grafana's API response structure.
22-23: ChangeUpdatedproperty to nullableDateTime?.The Grafana provisioning API documents the
updatedfield as optional. Rules that have never been modified may have a null or missingupdatedfield, causing JSON deserialisation to fail or produceDateTime.MinValue. Change toDateTime?to handle these cases correctly.
src/Runtime/StudioGateway/src/StudioGateway.Api/Authentication/GrafanaAuthenticationHandler.cs
Show resolved
Hide resolved
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/DesignerClient.cs
Outdated
Show resolved
Hide resolved
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/DesignerClientRegistration.cs
Outdated
Show resolved
Hide resolved
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs:
- Around line 67-70: Duplicate IKubernetes registration: a singleton Kubernetes
instance created with KubernetesClientConfiguration.BuildDefaultConfig() is
being registered via builder.Services.AddSingleton(...) while
AddKubernetesServices() later also registers IKubernetes as a singleton using
InClusterConfig(), causing the second to override the first. Fix by removing the
explicit builder.Services.AddSingleton(...) registration and rely on
AddKubernetesServices(), or if BuildDefaultConfig() is required, update
KubernetesServiceRegistration (the AddKubernetesServices() implementation) to
construct Kubernetes with BuildDefaultConfig() instead of InClusterConfig() and
delete the standalone AddSingleton registration so IKubernetes is registered
only once.
🧹 Nitpick comments (2)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/PodsClient.cs (1)
11-11: Method name is misleading.The method is named
GetReadyPodsCountAsyncbut returns a percentage (0–100) of ready pods, not an actual count. Consider renaming toGetReadyPodsPercentageAsyncfor clarity.src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs (1)
41-45: Consider adding URL validation.The Token is validated but the Url is not. Whilst
Urlis arequiredproperty, therequiredmodifier only enforces initialisation at compile-time for object initialisers—it doesn't prevent runtime binding from leaving it unset or malformed.🔧 Proposed fix
builder .Services.AddOptions<GrafanaSettings>() .Bind(builder.Configuration.GetSection("Grafana")) - .Validate(settings => !string.IsNullOrWhiteSpace(settings.Token), "Grafana settings validation failed. Grafana Token is required.") + .Validate(settings => !string.IsNullOrWhiteSpace(settings.Token), "Grafana settings validation failed. Grafana Token is required.") + .Validate(settings => settings.Url != null && settings.Url.IsAbsoluteUri, "Grafana settings validation failed. Grafana Url must be a valid absolute URI.") .ValidateOnStart();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Runtime/StudioGateway/Directory.Packages.propssrc/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/DesignerClient.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/DesignerClientRegistration.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/KubernetesServiceRegistration.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/PodsClient.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Program.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Settings/GrafanaSettings.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/StudioGateway.Api.csproj
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Runtime/StudioGateway/Directory.Packages.props
- src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/DesignerClient.cs
- src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/DesignerClientRegistration.cs
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
📚 Learning: 2025-12-11T09:10:06.960Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 17174
File: src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs:1-8
Timestamp: 2025-12-11T09:10:06.960Z
Learning: In C# 12, you can declare a class, interface, or struct with an empty body using a trailing semicolon as a shorthand (e.g., internal sealed class X : Y;). When reviewing StudioGateway.Api code under src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs and other C# files, verify the language version is set to at least 12.0+ and that any empty-body types use the single semicolon shorthand only when semantically appropriate (i.e., the type has no members). If a type previously had members, continue using a normal body.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/GrafanaSettings.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/PodsClient.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/KubernetesServiceRegistration.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
📚 Learning: 2025-01-28T22:18:03.328Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:43-48
Timestamp: 2025-01-28T22:18:03.328Z
Learning: Error handling improvements for the Get method in AzureDevOpsBuildClient were deemed out of scope for PR #14456, which focused on undeploy endpoints functionality.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/PodsClient.cs
📚 Learning: 2025-01-28T14:32:12.914Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/Repository/ORMImplementation/DeploymentRepository.cs:50-58
Timestamp: 2025-01-28T14:32:12.914Z
Learning: In the Altinn Studio project, methods retrieving deployment entities should fail fast (throw exceptions) when no matching records are found, rather than handling null cases. This is demonstrated in the `GetLastDeployed` method of `DeploymentRepository` which uses `FirstAsync()` instead of `FirstOrDefaultAsync()`.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/PodsClient.cs
📚 Learning: 2025-01-28T14:27:24.269Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/tests/Designer.Tests/Designer.Tests.csproj:36-36
Timestamp: 2025-01-28T14:27:24.269Z
Learning: The System.Linq.Dynamic.Core package reference in Designer.Tests.csproj was added to fix a vulnerability in a transitive dependency by explicitly specifying a secure version.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/StudioGateway.Api.csproj
📚 Learning: 2025-11-06T08:18:41.230Z
Learnt from: olavsorl
Repo: Altinn/altinn-studio PR: 16899
File: src/Designer/backend/src/Designer/Services/Implementation/RepositorySI.cs:260-265
Timestamp: 2025-11-06T08:18:41.230Z
Learning: In Altinn Studio, UseNullableReferenceTypes should be set to true for new projects created via CreateService in RepositorySI.cs, but should default to false for existing projects during migration in AltinnGitRepository.CreateNewAltinnStudioSettings. This ensures new projects adopt nullable reference types while existing projects maintain backward compatibility until explicitly migrated.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/StudioGateway.Api.csproj
📚 Learning: 2025-08-06T12:36:57.397Z
Learnt from: ErlingHauan
Repo: Altinn/altinn-studio PR: 15977
File: frontend/libs/studio-components-legacy/src/components/StudioCard/index.tsx:2-7
Timestamp: 2025-08-06T12:36:57.397Z
Learning: When a developer copies a component from `studio-components-legacy` to `studio-components` in the Altinn Studio repository and has not added a deprecation comment for the component in `studio-components-legacy`, suggest adding the deprecation comment in the PR review.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/StudioGateway.Api.csproj
📚 Learning: 2025-08-06T12:37:01.381Z
Learnt from: ErlingHauan
Repo: Altinn/altinn-studio PR: 15977
File: frontend/libs/studio-components-legacy/src/components/StudioCard/index.tsx:2-7
Timestamp: 2025-08-06T12:37:01.381Z
Learning: When reviewing PRs in the Altinn Studio repository where a developer copies a component from `studio-components-legacy` to `studio-components`, suggest adding a `deprecated` JSDoc comment to the component in `studio-components-legacy` if one hasn't been added. The deprecation comment should recommend using the component from `studio/components` instead.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/StudioGateway.Api.csproj
📚 Learning: 2025-09-25T08:50:58.936Z
Learnt from: Jondyr
Repo: Altinn/altinn-studio PR: 16431
File: src/Designer/frontend/packages/ux-editor/src/converters/formLayoutConverters/externalLayoutToInternal.ts:28-33
Timestamp: 2025-09-25T08:50:58.936Z
Learning: The Altinn Studio codebase contains two parallel UX editor packages: `ux-editor` (current/newer) and `ux-editor-v3` (legacy). When reviewing code changes, need to distinguish between these versions as they have different API implementations, particularly around datamodel binding conversions.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/StudioGateway.Api.csproj
📚 Learning: 2025-08-25T13:55:27.042Z
Learnt from: mgunnerud
Repo: Altinn/altinn-studio PR: 16187
File: frontend/resourceadm/components/ResourcePageInputs/ResourceSwitchInput.tsx:3-3
Timestamp: 2025-08-25T13:55:27.042Z
Learning: In the Altinn Studio codebase, when migrating from studio/components-legacy to studio/components, PRs should focus only on components that have available replacements in the new package. Components without new implementations should remain in the legacy package until replacements are available, supporting an incremental migration approach.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/StudioGateway.Api.csproj
📚 Learning: 2025-04-29T08:18:57.422Z
Learnt from: mgunnerud
Repo: Altinn/altinn-studio PR: 15272
File: backend/src/Designer/Models/ConsentMetadata.cs:0-0
Timestamp: 2025-04-29T08:18:57.422Z
Learning: In the Altinn Studio codebase, C# model classes use PascalCase for property names without explicit JsonPropertyName attributes. The application likely uses a global JSON serialization configuration that automatically handles case conversion between C# PascalCase and JavaScript camelCase.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cs
📚 Learning: 2025-01-28T22:20:43.402Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:70-75
Timestamp: 2025-01-28T22:20:43.402Z
Learning: The QueueBuildRequest class in Altinn Studio uses JsonPropertyName attributes from System.Text.Json for serialization (e.g., [JsonPropertyName("definition")] for the DefinitionReference property).
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cs
📚 Learning: 2025-04-29T08:18:57.422Z
Learnt from: mgunnerud
Repo: Altinn/altinn-studio PR: 15272
File: backend/src/Designer/Models/ConsentMetadata.cs:0-0
Timestamp: 2025-04-29T08:18:57.422Z
Learning: In the Altinn Studio codebase, C# model classes use PascalCase for property names without explicit JsonPropertyName attributes. The ASP.NET Core framework automatically handles serialization between C# PascalCase and JSON camelCase without requiring explicit property name mapping.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cs
📚 Learning: 2025-01-28T22:20:43.402Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:70-75
Timestamp: 2025-01-28T22:20:43.402Z
Learning: The QueueBuildRequest class in Altinn Studio uses System.Text.Json serialization annotations directly on the class properties, making explicit serializer options unnecessary.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cs
📚 Learning: 2025-11-25T08:20:59.697Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/test/**/*.cs : Use snapshot testing for OpenAPI docs and telemetry output
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cs
📚 Learning: 2025-11-25T08:20:59.697Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/*.cs : Register services in DI container properly following existing patterns for options configuration
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/KubernetesServiceRegistration.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
🧬 Code graph analysis (4)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/PodsClient.cs (2)
src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/GatewayContext.cs (1)
GatewayContext(3-9)src/Runtime/operator/internal/operatorcontext/operatorcontext.go (1)
ServiceOwner(32-36)
src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cs (5)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/Contracts/GrafanaAlertRule.cs (1)
GrafanaAlertRule(5-42)src/Runtime/StudioGateway/src/StudioGateway.Contracts/Alerts/AlertRule.cs (1)
AlertRule(5-36)src/Runtime/StudioGateway/src/StudioGateway.Contracts/Metrics/ErrorMetric.cs (1)
ErrorMetric(3-8)src/Runtime/StudioGateway/src/StudioGateway.Contracts/Metrics/AppErrorMetric.cs (1)
AppErrorMetric(3-7)src/Runtime/StudioGateway/src/StudioGateway.Contracts/Metrics/AppHealthMetric.cs (1)
AppHealthMetric(3-7)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/KubernetesServiceRegistration.cs (1)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/PodsClient.cs (1)
PodsClient(7-52)
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs (6)
src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/GrafanaSettings.cs (1)
GrafanaSettings(3-7)src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/AlertsClientSettings.cs (1)
AlertsClientSettings(3-6)src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/MetricsClientSettings.cs (1)
MetricsClientSettings(3-6)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/GrafanaClient.cs (1)
GrafanaClient(5-17)src/Runtime/StudioGateway/src/StudioGateway.Api/Authentication/GrafanaAuthenticationHandler.cs (2)
GrafanaAuthenticationHandler(6-29)GrafanaAuthenticationHandler(10-13)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs (1)
AzureMonitorClient(12-256)
🔇 Additional comments (6)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/K8s/KubernetesServiceRegistration.cs (1)
12-16: LGTM!The simplified
Kubernetesinstantiation and addition ofPodsClientas a singleton are correct.PodsClientappropriately usesIOptionsMonitor<GatewayContext>which supports dynamic configuration reload within singleton services.src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/GrafanaSettings.cs (1)
5-6: LGTM!Using
requiredmodifiers and changingUrlfromstringtoUriimproves type safety and ensures proper initialisation. This aligns well with the validation configured inProgram.cs.src/Runtime/StudioGateway/src/StudioGateway.Api/StudioGateway.Api.csproj (1)
18-21: LGTM!The Azure package additions are appropriate. Notably,
Azure.Monitor.Query.Logsis the correct replacement for the obsoleteAzure.Monitor.Querypackage. Versions are centrally managed viaDirectory.Packages.props.src/Runtime/StudioGateway/src/StudioGateway.Api/AppJsonSerializerContext.cs (1)
19-25: LGTM!The new
JsonSerializableattributes are required for AOT compilation support. All new contract types used in the alerts and metrics endpoints are properly registered.src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs (2)
22-32: LGTM!The Azure client registrations follow proper patterns:
TokenCredentialas singleton enables shared authentication, andLogsQueryClient/ArmClientcorrectly consume the credential via DI.
71-78: LGTM!The keyed service registrations for
IAlertsClientandIMetricsClientfollow the .NET 8+ keyed services pattern correctly, enabling provider-based resolution.Also applies to: 91-91
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs:
- Around line 41-45: The AddOptions<GrafanaSettings>() setup validates Token but
not Url; update the options registration to also validate that
GrafanaSettings.Url is not null/whitespace (and optionally well-formed) by
adding a Validate(...) call that checks !string.IsNullOrWhiteSpace(settings.Url)
(or Uri.IsWellFormedUriString(settings.Url, UriKind.Absolute) if you want
stricter validation) alongside the existing Token validation, so the
configuration for GrafanaSettings (used later when configuring the HttpClient)
cannot be null and won’t cause a NullReferenceException at runtime.
🧹 Nitpick comments (1)
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs (1)
67-87: Keyed service registrations and HttpClient configuration look good.The keyed DI registrations for
IAlertsClientandIMetricsClientfollow the correct pattern, and the HttpClient configuration now properly usesIOptionsMonitor<GrafanaSettings>.CurrentValueto support configuration reload (addressing previous review concerns).Optional observation: The
BaseAddressis set when the HttpClient is created from the factory (line 83), whilst theTokenis resolved per-request byGrafanaAuthenticationHandler. This means URL changes require waiting for HttpClient pool rotation (typically ~2 minutes), whilst Token changes take effect immediately. If immediate URL reload is important for your use case, consider resolving the base address in the handler as well. Otherwise, the current implementation is acceptable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
📚 Learning: 2025-11-25T08:20:59.697Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/*.cs : Register services in DI container properly following existing patterns for options configuration
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
📚 Learning: 2025-06-24T12:26:18.373Z
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15704
File: src/KubernetesWrapper/src/Program.cs:46-54
Timestamp: 2025-06-24T12:26:18.373Z
Learning: When security issues are identified that are out of scope for the current PR, mlqn prefers to create separate issues to track them rather than addressing them in the current PR. This was demonstrated when they created issue #15768 to handle CORS origin restrictions after it was flagged as a security concern in the KubernetesWrapper Program.cs file.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
📚 Learning: 2025-12-11T09:10:06.960Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 17174
File: src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs:1-8
Timestamp: 2025-12-11T09:10:06.960Z
Learning: In C# 12, you can declare a class, interface, or struct with an empty body using a trailing semicolon as a shorthand (e.g., internal sealed class X : Y;). When reviewing StudioGateway.Api code under src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs and other C# files, verify the language version is set to at least 12.0+ and that any empty-body types use the single semicolon shorthand only when semantically appropriate (i.e., the type has no members). If a type previously had members, continue using a normal body.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
🧬 Code graph analysis (1)
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs (7)
src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/GrafanaSettings.cs (1)
GrafanaSettings(3-7)src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/GatewayContext.cs (1)
GatewayContext(3-9)src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/AlertsClientSettings.cs (1)
AlertsClientSettings(3-6)src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/MetricsClientSettings.cs (1)
MetricsClientSettings(3-6)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/AlertsClient/GrafanaClient.cs (1)
GrafanaClient(5-17)src/Runtime/StudioGateway/src/StudioGateway.Api/Authentication/GrafanaAuthenticationHandler.cs (2)
GrafanaAuthenticationHandler(6-29)GrafanaAuthenticationHandler(10-13)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs (1)
AzureMonitorClient(12-256)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test
🔇 Additional comments (2)
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs (2)
22-32: LGTM! Azure SDK clients properly configured.The registration of
DefaultAzureCredential,LogsQueryClient, andArmClientas singletons follows Azure SDK best practices. The credential will fail fast at startup if authentication is unavailable, ensuring early detection of configuration issues.
46-55: LGTM! Options pattern correctly implemented with reload support.The configuration bindings and scoped registrations using
IOptionsSnapshotproperly enable dynamic configuration reload whilst avoiding the verbosity ofIOptions<T>throughout the codebase. The use ofTryAddScopedis appropriately defensive.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs:
- Around line 48-49: Validate configured provider values after binding
AlertsClientSettings and MetricsClientSettings to ensure they match the keyed
service registrations: check that AlertsClientSettings.Provider equals the
registered key "grafana" for IAlertsClient and MetricsClientSettings.Provider
equals "azuremonitor" for IMetricsClient (or validate both against a known
providers list), and throw a clear exception during startup if mismatched;
perform this check immediately after calling
builder.Services.Configure<AlertsClientSettings>(...) and
Configure<MetricsClientSettings>(...) so misconfiguration fails fast with a
descriptive error referencing AlertsClientSettings.Provider and
MetricsClientSettings.Provider and the expected keys ("grafana",
"azuremonitor").
🧹 Nitpick comments (1)
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs (1)
22-32: Consider configuring DefaultAzureCredential explicitly.
DefaultAzureCredentialuses a credential chain that attempts multiple authentication methods. For production environments, explicitly configuring which credential type to use (e.g.,ManagedIdentityCredentialfor Azure-hosted services) can improve startup performance and provide clearer error messages if authentication fails.💡 Example: Explicit credential configuration
-builder.Services.AddSingleton<TokenCredential, DefaultAzureCredential>(); +builder.Services.AddSingleton<TokenCredential>(sp => +{ + var options = new DefaultAzureCredentialOptions + { + ExcludeVisualStudioCredential = true, + ExcludeVisualStudioCodeCredential = true, + ExcludeAzureCliCredential = true, + ExcludeInteractiveBrowserCredential = true + }; + return new DefaultAzureCredential(options); +});This restricts the credential chain to environment variables, managed identity, and workload identity—appropriate for containerised production environments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-25T08:20:59.697Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/*.cs : Register services in DI container properly following existing patterns for options configuration
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
📚 Learning: 2025-06-24T12:26:18.373Z
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15704
File: src/KubernetesWrapper/src/Program.cs:46-54
Timestamp: 2025-06-24T12:26:18.373Z
Learning: When security issues are identified that are out of scope for the current PR, mlqn prefers to create separate issues to track them rather than addressing them in the current PR. This was demonstrated when they created issue #15768 to handle CORS origin restrictions after it was flagged as a security concern in the KubernetesWrapper Program.cs file.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
📚 Learning: 2025-12-11T09:10:06.960Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 17174
File: src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs:1-8
Timestamp: 2025-12-11T09:10:06.960Z
Learning: In C# 12, you can declare a class, interface, or struct with an empty body using a trailing semicolon as a shorthand (e.g., internal sealed class X : Y;). When reviewing StudioGateway.Api code under src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs and other C# files, verify the language version is set to at least 12.0+ and that any empty-body types use the single semicolon shorthand only when semantically appropriate (i.e., the type has no members). If a type previously had members, continue using a normal body.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
🧬 Code graph analysis (1)
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs (6)
src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/GrafanaSettings.cs (1)
GrafanaSettings(3-7)src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/GatewayContext.cs (1)
GatewayContext(3-9)src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/AlertsClientSettings.cs (1)
AlertsClientSettings(3-6)src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/MetricsClientSettings.cs (1)
MetricsClientSettings(3-6)src/Runtime/StudioGateway/src/StudioGateway.Api/Authentication/GrafanaAuthenticationHandler.cs (2)
GrafanaAuthenticationHandler(6-29)GrafanaAuthenticationHandler(10-13)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs (1)
AzureMonitorClient(12-256)
🔇 Additional comments (6)
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs (6)
40-46: LGTM: Grafana settings properly validated.The Options pattern is correctly configured with validation for both
TokenandUrlproperties, andValidateOnStartensures the application fails fast if the configuration is invalid.
53-56: Settings registration pattern is acceptable.Registering settings classes directly as scoped services (resolving from
IOptionsSnapshot<T>.Value) is a reasonable pattern that simplifies injection whilst supporting dynamic configuration reload. The comment clearly documents the intent.
68-75: LGTM: Keyed service registration is correct.The keyed transient registration for
IAlertsClientcorrectly uses theIHttpClientFactoryto create aGrafanaClientwith the named HttpClient, following best practices for HTTP client management.
76-77: LGTM: Required services for authentication handler.
AddHttpContextAccessorand the transientGrafanaAuthenticationHandlerregistration are correctly configured to support per-request authentication token injection.
78-87: HttpClient configuration: Token is dynamic, BaseAddress is semi-static.The current implementation uses
IOptionsMonitor<GrafanaSettings>.CurrentValueto setBaseAddressat client creation, whilstGrafanaAuthenticationHandlerresolves the token per-request. This means:
- ✅ Token rotations are immediately effective (per-request resolution)
⚠️ URL changes won't apply to existing HttpClient instances (pooled clients are reused)However, HttpClientFactory automatically expires and recreates clients after ~2 minutes, so URL changes will eventually take effect without restart. Given that URLs rarely change in production (compared to credential rotation), and the startup validation ensures the URL is initially correct, this trade-off is acceptable for most scenarios.
If immediate URL updates without restart are required, consider implementing a
DelegatingHandlerthat sets the base address per-request (similar to the authentication handler's approach with tokens).
88-88: LGTM: Metrics client keyed registration.The keyed transient registration for
IMetricsClientwithAzureMonitorClientis correctly configured and consistent with the alerts client registration pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/DesignerClient.cs:
- Around line 14-16: The method parameter name environement in
NotifyAlertsUpdatedAsync is misspelled; rename it to environment in the method
signature and update all references inside the method (e.g., the
httpClientFactory.CreateClient(environement) call) to use the corrected name,
and update any callers to pass the renamed parameter where needed so signatures
remain consistent.
🧹 Nitpick comments (2)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/DesignerClient.cs (1)
13-13: Remove spurious<inheritdoc />.This class does not implement an interface, so
<inheritdoc />has no effect. Either remove the doc comment entirely or replace it with meaningful documentation for this method.🔧 Proposed fix
- /// <inheritdoc /> + /// <summary> + /// Notifies the Designer service that alerts have been updated for the current service owner and environment. + /// </summary> public async Task NotifyAlertsUpdatedAsync(string environement, CancellationToken cancellationToken)src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs (1)
46-54: Consider adding provider validation.Both
AlertsClientSettingsandMetricsClientSettingsuseValidateOnStart()but have no explicitValidate()call for theProviderproperty. While the single-member enums currently prevent invalid values from binding successfully, adding explicit validation would provide clearer error messages and future-proof against enum expansion.🔧 Optional: Add explicit provider validation
builder .Services.AddOptions<AlertsClientSettings>() .Bind(builder.Configuration.GetSection("AlertsClientSettings")) + .Validate( + s => Enum.IsDefined(typeof(AlertsClientSettings.AlertsClientProvider), s.Provider), + "AlertsClientSettings.Provider must be a valid provider." + ) .ValidateOnStart(); builder .Services.AddOptions<MetricsClientSettings>() .Bind(builder.Configuration.GetSection("MetricsClientSettings")) + .Validate( + s => Enum.IsDefined(typeof(MetricsClientSettings.MetricsClientProvider), s.Provider), + "MetricsClientSettings.Provider must be a valid provider." + ) .ValidateOnStart();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/DesignerClient.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Program.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Settings/AlertsClientSettings.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Settings/MetricsClientSettings.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/appsettings.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Runtime/StudioGateway/src/StudioGateway.Api/appsettings.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-11T09:10:06.960Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 17174
File: src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs:1-8
Timestamp: 2025-12-11T09:10:06.960Z
Learning: In C# 12, you can declare a class, interface, or struct with an empty body using a trailing semicolon as a shorthand (e.g., internal sealed class X : Y;). When reviewing StudioGateway.Api code under src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs and other C# files, verify the language version is set to at least 12.0+ and that any empty-body types use the single semicolon shorthand only when semantically appropriate (i.e., the type has no members). If a type previously had members, continue using a normal body.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/MetricsClientSettings.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/DesignerClient.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Settings/AlertsClientSettings.cssrc/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
📚 Learning: 2025-11-25T08:20:59.697Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/*.cs : Register services in DI container properly following existing patterns for options configuration
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
📚 Learning: 2025-06-24T12:26:18.373Z
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15704
File: src/KubernetesWrapper/src/Program.cs:46-54
Timestamp: 2025-06-24T12:26:18.373Z
Learning: When security issues are identified that are out of scope for the current PR, mlqn prefers to create separate issues to track them rather than addressing them in the current PR. This was demonstrated when they created issue #15768 to handle CORS origin restrictions after it was flagged as a security concern in the KubernetesWrapper Program.cs file.
Applied to files:
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs
🧬 Code graph analysis (2)
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/DesignerClient.cs (2)
src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/GatewayContext.cs (1)
GatewayContext(3-9)src/Runtime/StudioGateway/src/StudioGateway.Api/Application/HandleAlerts.cs (2)
Task(11-39)Task(41-49)
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs (5)
src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/GrafanaSettings.cs (1)
GrafanaSettings(3-7)src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/AlertsClientSettings.cs (1)
AlertsClientSettings(3-11)src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/MetricsClientSettings.cs (1)
MetricsClientSettings(3-11)src/Runtime/StudioGateway/src/StudioGateway.Api/Authentication/GrafanaAuthenticationHandler.cs (2)
GrafanaAuthenticationHandler(6-29)GrafanaAuthenticationHandler(10-13)src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/MetricsClient/AzureMonitorClient.cs (1)
AzureMonitorClient(12-256)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test
🔇 Additional comments (4)
src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/MetricsClientSettings.cs (1)
1-11: LGTM!The settings class follows the same pattern as
AlertsClientSettingsand correctly uses a required property to ensure the provider is configured. The nested enum keeps the provider scope tightly coupled to its settings class.src/Runtime/StudioGateway/src/StudioGateway.Api/Settings/AlertsClientSettings.cs (1)
1-11: LGTM!The settings class structure is appropriate for a provider-based configuration pattern. The required property ensures the provider must be specified in configuration.
src/Runtime/StudioGateway/src/StudioGateway.Api/Program.cs (2)
21-31: Azure SDK client registrations look correct.The
TokenCredentialsingleton pattern withDefaultAzureCredentialis the recommended approach for Azure SDK clients. BothLogsQueryClientandArmClientcorrectly receive the shared credential.
74-96: Keyed service wiring is well-structured.Using the enum's
ToString()as the service key ensures consistency between registration and resolution. TheGrafanaAuthenticationHandlercorrectly handles per-request token resolution, whilst theBaseAddressis set from validated settings.One consideration:
httpClient.BaseAddressis set once perHttpClientinstance. If Grafana URL rotation is required without restart, a customDelegatingHandlerthat sets the request URI dynamically would be needed. For most deployments, this is acceptable.
src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/DesignerClient.cs
Outdated
Show resolved
Hide resolved
|
Hmm is the test failure consistent? Maybe catch up with main. If it still fails I can take a look if you want |
2 participants
Description
Add errors, health and app metrics in Gateway API
Verification
Summary by CodeRabbit
New Features
Integrations
Chores
✏️ Tip: You can customize this high-level summary in your review settings.