feat(devices): device agent logs API backed by Loki - #2188
aliaska-varieva wants to merge 3 commits into
Conversation
Add the openframe-data-loki module (Loki query client over Spring's RestClient HTTP interface, LogQL escaping) and a deviceLogs GraphQL query that reads the agent-logs streams written by openframe-saas-logs-stream. The stream selector is pinned to the tenant's own domain from the tenants collection and the device must be visible to the tenant. Results are newest first with a timestamp+skip cursor. Disabled unless openframe.loki.enabled=true.
🦩 Flamingo Code Review5 finding(s) — 5 action required · 0 recommended · 0 informational Mode: advisory · Rules cited: Inline comments: 5 new Need another pass? Commits pushed after this review are not reviewed automatically.
Prefer typing? Comment React 👍/👎 on inline comments to teach the reviewer. Started 2026-09-14 18:51 UTC · updated 2026-09-14 18:53 UTC · workflow run |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| public record LokiLogEntry(long timestampNanos, String line, Map<String, String> labels) { | ||
|
|
||
| public Instant timestamp() { | ||
| return Instant.ofEpochSecond(0, timestampNanos); | ||
| } | ||
| } |
There was a problem hiding this comment.
🦩 🔴 [error/action_required] OFJAVA-033 LokiLogEntry declared as a Java record, violating the no-records convention
OFJAVA-033 forbids Java record declarations in favor of Lombok-annotated classes. This new file declares public record LokiLogEntry(...), which is a direct violation. Additionally OFJAVA-035/OFJAVA-013 guidance around clean data classes point to using @Data/@Getter/@AllArgsConstructor style classes instead. Convert to a Lombok class with @Getter/@AllArgsConstructor (or equivalent) per the project convention.
Evidence
public record LokiLogEntry(long timestampNanos, String line, Map<String, String> labels) {
public Instant timestamp() {
return Instant.ofEpochSecond(0, timestampNanos);
}
}
🤖 Prompt for AI agents
In openframe-data-loki/src/main/java/com/openframe/data/loki/model/LokiLogEntry.java around lines 9-14, address this code-review finding: LokiLogEntry declared as a Java record, violating the no-records convention.
OFJAVA-033 forbids Java `record` declarations in favor of Lombok-annotated classes. This new file declares `public record LokiLogEntry(...)`, which is a direct violation. Additionally OFJAVA-035/OFJAVA-013 guidance around clean data classes point to using @Data/@Getter/@AllArgsConstructor style classes instead. Convert to a Lombok class with @Getter/@AllArgsConstructor (or equivalent) per the project convention.
The flagged code:
```
public record LokiLogEntry(long timestampNanos, String line, Map<String, String> labels) {
public Instant timestamp() {
return Instant.ofEpochSecond(0, timestampNanos);
}
}
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 85 — react 👍/👎 to teach the reviewer
| public record LokiQueryResponse(String status, QueryData data) { | ||
|
|
||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public record QueryData(String resultType, List<LogStream> result) { | ||
| } | ||
|
|
||
| /** | ||
| * One log stream. Without the categorize-labels encoding flag Loki folds structured metadata into | ||
| * {@code stream}, so each distinct metadata set arrives as its own stream. Each value is a | ||
| * {@code [timestampNanos, line]} pair. | ||
| */ | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public record LogStream(Map<String, String> stream, List<List<String>> values) { | ||
| } | ||
| } |
There was a problem hiding this comment.
🦩 🔴 [error/action_required] OFJAVA-033 LokiQueryResponse and nested types declared as Java records
LokiQueryResponse, its nested QueryData, and LogStream are all declared using record, which OFJAVA-033 explicitly forbids project-wide in favor of Lombok-annotated classes (@Data/@Getter/@Builder/@AllArgsConstructor/@NoArgsConstructor). This is a new file introducing three record violations at once; it should be rewritten as nested Lombok classes to match the codebase convention (and to be consistent with OPENFRAM-003-3's nested-static-class-with-full-Lombok-quartet pattern for structured DTOs).
Evidence
public record LokiQueryResponse(String status, QueryData data) {
@JsonIgnoreProperties(ignoreUnknown = true)
public record QueryData(String resultType, List<LogStream> result) {
}
/**
* One log stream. Without the categorize-labels encoding flag Loki folds structured metadata into
* {@code stream}, so each distinct metadata set arrives as its own stream. Each value is a
* {@code [timestampNanos, line]} pair.
*/
@JsonIgnoreProperties(ignoreUnknown = true)
🤖 Prompt for AI agents
In openframe-data-loki/src/main/java/com/openframe/data/loki/model/LokiQueryResponse.java around lines 9-23, address this code-review finding: LokiQueryResponse and nested types declared as Java records.
LokiQueryResponse, its nested QueryData, and LogStream are all declared using `record`, which OFJAVA-033 explicitly forbids project-wide in favor of Lombok-annotated classes (@Data/@Getter/@Builder/@AllArgsConstructor/@NoArgsConstructor). This is a new file introducing three record violations at once; it should be rewritten as nested Lombok classes to match the codebase convention (and to be consistent with OPENFRAM-003-3's nested-static-class-with-full-Lombok-quartet pattern for structured DTOs).
The flagged code:
```
public record LokiQueryResponse(String status, QueryData data) {
@JsonIgnoreProperties(ignoreUnknown = true)
public record QueryData(String resultType, List<LogStream> result) {
}
/**
* One log stream. Without the categorize-labels encoding flag Loki folds structured metadata into
* {@code stream}, so each distinct metadata set arrives as its own stream. Each value is a
* {@code [timestampNanos, line]} pair.
*/
@JsonIgnoreProperties(ignoreUnknown = true)
public record LogStream(Map<String, String> stream, List<List<String>> values) {
}
}
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 85 — react 👍/👎 to teach the reviewer
| * entries at exactly that timestamp have been returned so far. Loki has no offsets, so the next page is read | ||
| * up to and including that timestamp and the already returned entries at it are skipped. | ||
| */ | ||
| record DeviceLogCursor(long timestampNanos, int skip) { |
There was a problem hiding this comment.
🦩 🔴 [error/action_required] OFJAVA-033 DeviceLogCursor declared as a Java record, violating the no-record convention
OFJAVA-033 forbids Java record types in this codebase in favor of Lombok-annotated classes. DeviceLogCursor is declared as record DeviceLogCursor(long timestampNanos, int skip), which is a direct violation. It should be rewritten as a class using @Getter/@AllArgsConstructor (or similar) per the established convention, keeping the same accessor names (timestampNanos(), skip()) or updating call sites accordingly.
Evidence
record DeviceLogCursor(long timestampNanos, int skip) {
🤖 Prompt for AI agents
In openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceLogCursor.java around line 10, address this code-review finding: DeviceLogCursor declared as a Java record, violating the no-record convention.
OFJAVA-033 forbids Java `record` types in this codebase in favor of Lombok-annotated classes. `DeviceLogCursor` is declared as `record DeviceLogCursor(long timestampNanos, int skip)`, which is a direct violation. It should be rewritten as a class using @Getter/@AllArgsConstructor (or similar) per the established convention, keeping the same accessor names (timestampNanos(), skip()) or updating call sites accordingly.
The flagged code:
```
record DeviceLogCursor(long timestampNanos, int skip) {
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 75 — react 👍/👎 to teach the reviewer
| List<LokiLogEntry> entries = lokiClient.queryRange(query, startNanos, endNanos, pageSize + skip + 1, | ||
| LokiDirection.BACKWARD); | ||
| List<LokiLogEntry> remaining = dropReturned(entries, after); | ||
|
|
||
| return result(toItems(remaining.subList(0, Math.min(pageSize, remaining.size())), after), | ||
| remaining.size() > pageSize, after != null); |
There was a problem hiding this comment.
🦩 🔴 [error/action_required] OFJAVA-002 Nested method calls passed directly as arguments in DeviceLogService.queryDeviceLogs
OFJAVA-002 (CI-enforced by PMD's NoMethodCallAsArgument) requires extracting method calls to named locals before passing them as arguments. Several call sites in DeviceLogService pass method-call results directly, e.g. lokiClient.queryRange(query, startNanos, endNanos, pageSize + skip + 1, LokiDirection.BACKWARD) and result(toItems(remaining.subList(...), after), remaining.size() > pageSize, after != null). These nested calls should be extracted into named locals (e.g. int queryLimit = pageSize + skip + 1;, List<DeviceLogEntry> items = toItems(...);) to satisfy the CI-enforced rule.
Evidence
List<LokiLogEntry> entries = lokiClient.queryRange(query, startNanos, endNanos, pageSize + skip + 1,
LokiDirection.BACKWARD);
List<LokiLogEntry> remaining = dropReturned(entries, after);
return result(toItems(remaining.subList(0, Math.min(pageSize, remaining.size())), after),
remaining.size() > pageSize, after != null);
🤖 Prompt for AI agents
In openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceLogService.java around lines 93-98, address this code-review finding: Nested method calls passed directly as arguments in DeviceLogService.queryDeviceLogs.
OFJAVA-002 (CI-enforced by PMD's NoMethodCallAsArgument) requires extracting method calls to named locals before passing them as arguments. Several call sites in DeviceLogService pass method-call results directly, e.g. `lokiClient.queryRange(query, startNanos, endNanos, pageSize + skip + 1, LokiDirection.BACKWARD)` and `result(toItems(remaining.subList(...), after), remaining.size() > pageSize, after != null)`. These nested calls should be extracted into named locals (e.g. `int queryLimit = pageSize + skip + 1;`, `List<DeviceLogEntry> items = toItems(...);`) to satisfy the CI-enforced rule.
The flagged code:
```
List<LokiLogEntry> entries = lokiClient.queryRange(query, startNanos, endNanos, pageSize + skip + 1,
LokiDirection.BACKWARD);
List<LokiLogEntry> remaining = dropReturned(entries, after);
return result(toItems(remaining.subList(0, Math.min(pageSize, remaining.size())), after),
remaining.size() > pageSize, after != null);
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 55 — react 👍/👎 to teach the reviewer
| private Duration connectTimeout = Duration.ofSeconds(2); | ||
|
|
||
| /** | ||
| * Upper bound for one query. These are user-facing reads, so it stays below Loki's own query timeout. | ||
| */ | ||
| private Duration readTimeout = Duration.ofSeconds(30); |
There was a problem hiding this comment.
🦩 🔴 [error/action_required] OFJAVA-011 LokiProperties fields use inline defaults instead of failing fast on missing config, violating no-inline-defaults rule
OFJAVA-011 requires @value properties to have no inline defaults so that missing configuration fails fast per environment. While this class uses @ConfigurationProperties (which the rule explicitly prefers over @value 'for grouped configuration with validation'), it still hardcodes default values directly on the fields (connectTimeout = Duration.ofSeconds(2), readTimeout = Duration.ofSeconds(30)) rather than requiring explicit configuration per environment or validating via @ConfigurationProperties validation annotations (e.g. @NotNull with a @validated config class). This masks configuration drift between environments — e.g., prod could silently run with a 2s connect timeout intended only for local dev. Consider adding explicit @validated + @NotNull constraints, or requiring the values to be set explicitly per environment.
Evidence
private Duration connectTimeout = Duration.ofSeconds(2);
/**
* Upper bound for one query. These are user-facing reads, so it stays below Loki's own query timeout.
*/
private Duration readTimeout = Duration.ofSeconds(30);
🤖 Prompt for AI agents
In openframe-data-loki/src/main/java/com/openframe/data/loki/config/LokiProperties.java around lines 25-30, address this code-review finding: LokiProperties fields use inline defaults instead of failing fast on missing config, violating no-inline-defaults rule.
OFJAVA-011 requires @Value properties to have no inline defaults so that missing configuration fails fast per environment. While this class uses @ConfigurationProperties (which the rule explicitly prefers over @Value 'for grouped configuration with validation'), it still hardcodes default values directly on the fields (connectTimeout = Duration.ofSeconds(2), readTimeout = Duration.ofSeconds(30)) rather than requiring explicit configuration per environment or validating via @ConfigurationProperties validation annotations (e.g. @NotNull with a @Validated config class). This masks configuration drift between environments — e.g., prod could silently run with a 2s connect timeout intended only for local dev. Consider adding explicit @Validated + @NotNull constraints, or requiring the values to be set explicitly per environment.
The flagged code:
```
private Duration connectTimeout = Duration.ofSeconds(2);
/**
* Upper bound for one query. These are user-facing reads, so it stays below Loki's own query timeout.
*/
private Duration readTimeout = Duration.ofSeconds(30);
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 35 — react 👍/👎 to teach the reviewer
A tenant pod serves one tenant and tenant domains never change, so resolve the domain from Mongo once per pod instead of on every deviceLogs request. A missing domain is not cached, so a tenant still being provisioned recovers on the next call.
…l-Loki tests Serve up to 500 lines per deviceLogs page (default 100): the agent ships up to 50 lines a minute per device, so the shared 100-item cap covered only a couple of minutes of a busy device. Pages no longer split lines that share a timestamp. Loki cuts a result at the limit without regard to timestamps, so the previous skip-based cursor could drop or repeat lines at a shared timestamp. The cursor is now the last line's timestamp and the next page ends exactly at it. Add LokiClientIT and DeviceLogServiceIT, which run against Loki 3.7.3 in Testcontainers during the normal test phase.
1 participant
Summary
openframe-data-lokimodule: Loki query client built on a Spring HTTP interface over Boot'sRestClient.Builder(standardhttp.client.requestsmetrics/tracing, connect/read timeouts),LogQlliteral escaping,LokiQueryException. There is no maintained Java Loki query library (loki4j only pushes).DeviceLogService: reads{job="agent-logs", tenant_domain="…"} | machine_id="…", the same streams the Grafana Tenant Clients dashboard uses (written byopenframe-saas-logs-stream).deviceLogs(machineId, filter, first, after), newest first. Filter:levels,search(case-insensitive substring),from/to(default last 7 days, max 30).LOKI_QUERY_ERROR(503) mapped inGraphQLExceptionHandler.openframe.loki.enabled=true+openframe.loki.url, so OSS is unaffected.openframe.loki.*avoids the existing OSSloki.url(logback push appender).Tenant isolation
TenantIdProvider(openframe.cluster-id= the namespace'sTENANT_IDUUID) →tenants._id→domain. Nothing from the request is used, and the JWT has notenant_domainclaim.DeviceService, otherwiseDEVICE_NOT_FOUND.LogQl.quote/LogQl.regexLiteral.Pagination
Loki has no offsets. The cursor is the last returned line's nanosecond timestamp, and the next page ends exactly at it (Loki's
endis exclusive).Loki cuts a result at
limitwithout regard to timestamps and doesn't guarantee which of the lines sharing the cut timestamp it keeps, so a page never splits a timestamp: it ends before the cut timestamp and the next page starts with all of its lines. A page can therefore hold a few lines fewer thanfirstwhen lines share a timestamp at the boundary. A timestamp with more lines than a whole page is returned in full (up to 5,000). Undecodable cursors are rejected instead of silently restarting at page one.An earlier version skipped already-returned lines at the cursor timestamp; the real-Loki integration test below showed it could drop or repeat lines at a shared timestamp, which is what this design fixes.
Testing
*ITsuites:LokiClientIT:+, quotes and braces survive encoding; streams merge newest/oldest first with structured metadata;startinclusive /endexclusive; rejected queries becomeLokiQueryException.DeviceLogServiceIT, seeded the wayopenframe-saas-logs-streamwrites (labels + structured metadata): every line of the device and none from another device or tenant; 2-line pages across five lines sharing one nanosecond match a single query with no gaps or duplicates; 500-line cap and 100 default; level, search and window filters; search matched literally (hostile and regex-looking input); polling withfrom= newest line.LogQlTest,LokiClientTest,DeviceLogServiceTest,GraphQLDeviceLogMapperTest. ExistingDeviceDataFetcherTeststill passes.Rollout
Config for dev/qa/stage/prod: https://github.com/flamingo-stack/openframe-saas-tenant/pull/3181. It is inert until this lib version is bumped there.