feat(devices): device agent logs API backed by Loki - #2188
aliaska-varieva wants to merge 7 commits into
Conversation
🦩 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
There was a problem hiding this comment.
Thumbs-down on this and the two sibling OFJAVA-033 findings (LokiQueryResponse, DeviceLogCursor), because the premise doesn't hold in this repository:
- On
main, 57 files undersrc/maindeclare records today — including the External API request DTOs, which are records by the documented API pattern. - Records are still being added: several commits merged this month add new ones, one of them in a commit whose subject is
fix(OFJAVA-018). So the rule family is in active use while records keep landing. - Nothing enforces it mechanically: neither PMD nor Checkstyle is configured in the build or in CI.
The three flagged types are exactly what records are for: the Loki JSON response shape, one immutable log line, and a pagination cursor. None of them needs Lombok's mutability or builders.
If the no-records convention is genuinely new and intended going forward, say so and I'll convert all three to @Getter/@AllArgsConstructor classes — it's a few minutes of work. I'd rather confirm than churn the code against the grain of the codebase.
| 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
There was a problem hiding this comment.
Applied in 838dd6e. The nested calls now sit in named locals:
DeviceLogService:hasNextPage/hasPreviousPagebefore building the result, andatLeastOneLinein the page-size clamp.LokiClient:bodyfor the abbreviated Loki error, andtimestampNanoswhen building an entry.
All 39 unit and integration tests still pass, including the two suites that run against a real Loki, so this is a readability change only.
One correction for the rule text: the finding says this is "CI-enforced by PMD's NoMethodCallAsArgument", but neither PMD nor Checkstyle is configured in this repository's build or CI, so nothing was failing. The suggestion reads better regardless, which is why I took it.
| 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
There was a problem hiding this comment.
Thumbs-down: the setting that could actually drift between environments already fails fast.
openframe.loki.urlhas no default.LokiConfigasserts it is present whenopenframe.loki.enabled=true, so a misconfigured environment fails at startup rather than silently querying the wrong host.connect-timeout/read-timeoutare client tuning rather than per-environment configuration, and they are overridable per environment when needed. Spring Boot ships its own HTTP client timeouts the same way.- The rule text quoted here prefers
@ConfigurationProperties"for grouped configuration with validation", which is what this class is.
Flagged at confidence 35, and I think that's the right read. Happy to move the timeouts into configs/base as explicit values if you'd prefer every knob spelled out per environment.
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.
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 (CU-86agb21qt) 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.
…(CU-86agb21qt) Replace the single search filter with contains (every term must appear), excludes (no term may appear) and an opt-in regex. Each becomes its own line filter ahead of the machine_id filter, so the cheapest stage runs first. Terms are escaped and matched literally. Patterns are compiled here and rejected when they use lookaround or backreferences, which Loki's RE2 engine cannot run, so a mistyped pattern reads as a bad request instead of a failed query. Limits: 5 terms per list, 256 characters each.
fbe6513 to
9ef63de
Compare
… path (CU-86agb21qt) Extract hasNextPage/hasPreviousPage, the page-size clamp, the abbreviated Loki error body and the entry timestamp into named locals instead of nesting the calls in arguments, per review feedback (OFJAVA-002). No behaviour change.
…the budget (CU-86agb21qt) Java compiles an atomic group but Loki's RE2 engine rejects it, so `(?>...)` reached Loki and came back as a failed query instead of a bad request; it now fails validation like lookaround and backreferences. Blank terms are skipped when the line filters are built, so they no longer count towards the five-term limit either.
2 participants
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,contains/excludes/regex,from/to(default last 7 days, max 30).containsrequires every term (case-insensitive, matched literally),excludesrejects lines holding any term, andregexis an opt-in RE2 pattern. Each becomes its own line filter ahead of themachine_idfilter, so the cheapest stage runs first and the tenant selector is untouchable. Limits: 5 terms per list, 256 characters each; patterns using lookaround or backreferences, which Loki's RE2 engine cannot run, are rejected as a bad request instead of failing at Loki.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; several terms, exclusions and a regex pattern; search matched literally (hostile and regex-looking input); polling withfrom= newest line.LogQlTest,LokiClientTest,DeviceLogServiceTest(including the exact LogQL built from terms, exclusions and a pattern, and the rejected patterns),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.