Skip to content

Conversation

@ebyhr
Copy link
Member

@ebyhr ebyhr commented Nov 3, 2025

Description

The 1st commit comes from #27199

Fixes #26500, Fixes #27180

Release notes

## Loki connector
* Fix failure when initializing the connector. ({issue}`27180`)

@cla-bot cla-bot bot added the cla-signed label Nov 3, 2025
@sourcery-ai

This comment was marked as spam.

@github-actions github-actions bot added the loki Loki connector label Nov 3, 2025
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes - here's some feedback:

  • The TestLoki integration test may be flaky if logs aren’t ingested immediately; consider adding a retry loop or wait condition before querying for results.
  • To avoid interference between test runs, use a unique label or clear existing logs in Loki so that concurrent tests don’t pick up stale entries.
  • Instead of hardcoding the Grafana Loki Docker image version in EnvMultinodeLoki, consider centralizing the version in a configuration constant for easier upgrades.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The TestLoki integration test may be flaky if logs aren’t ingested immediately; consider adding a retry loop or wait condition before querying for results.
- To avoid interference between test runs, use a unique label or clear existing logs in Loki so that concurrent tests don’t pick up stale entries.
- Instead of hardcoding the Grafana Loki Docker image version in EnvMultinodeLoki, consider centralizing the version in a configuration constant for easier upgrades.

## Individual Comments

### Comment 1
<location> `testing/trino-product-tests/src/main/java/io/trino/tests/product/loki/TestLoki.java:51-56` </location>
<code_context>
+        client.pushLogLine("line 3", end.minus(Duration.ofMinutes(1)), ImmutableMap.of("test", "logs_query"));
+        client.flush();
+
+        assertThat(onTrino().executeQuery("SELECT value FROM TABLE(loki.system.query_range(" +
+                                          "'{test=\"logs_query\"}'," +
+                                          "TIMESTAMP '" + TIMESTAMP_FORMATTER.format(start) + "'," +
+                                          "TIMESTAMP '" + TIMESTAMP_FORMATTER.format(end) + "'))" +
+                                          "LIMIT 1"))
+                .containsOnly(row("line 1"));
+    }
+}
</code_context>

<issue_to_address>
**suggestion (testing):** Suggestion to add assertions for all returned log lines, not just the first.

Adding assertions for all expected log lines will improve test coverage and ensure the query_range function returns the complete and correctly ordered results.

```suggestion
        assertThat(onTrino().executeQuery("SELECT value FROM TABLE(loki.system.query_range(" +
                                          "'{test=\"logs_query\"}'," +
                                          "TIMESTAMP '" + TIMESTAMP_FORMATTER.format(start) + "'," +
                                          "TIMESTAMP '" + TIMESTAMP_FORMATTER.format(end) + "'))"))
                .containsExactly(
                        row("line 1"),
                        row("line 2"),
                        row("line 3")
                );
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@ebyhr ebyhr force-pushed the ebi/loki-product-test branch from 1061d29 to 12cbfa6 Compare November 3, 2025 23:49
private static final DateTimeFormatter TIMESTAMP_FORMATTER = DateTimeFormatter.ofPattern("uuuu-MM-dd HH:mm:ss.SSS'Z'").withZone(UTC);

@Test(groups = {LOKI, PROFILE_SPECIFIC_TESTS})
public void testQueryRange()
Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/trinodb/trino/actions/runs/19052975008/job/54421272410?pr=27201

tests               | 2025-11-04 06:49:08 INFO: SUCCESS     /    io.trino.tests.product.loki.TestLoki.testQueryRange (Groups: profile_specific_tests, loki) took 1.4 seconds
tests               | 2025-11-04 06:49:08 INFO: 
tests               | 2025-11-04 06:49:08 INFO: Completed 2 tests
tests               | 2025-11-04 06:49:08 INFO: 2 SUCCEEDED      /      0 FAILED      /      0 SKIPPED
tests               | 2025-11-04 06:49:08 INFO: Tests execution took 4.7 seconds
tests               | 2025-11-04 06:49:08 INFO: ManageTestResources.onFinish: running checks
tests               | 
tests               | ===============================================
tests               | tempto-tests
tests               | Total tests run: 2, Failures: 0, Skips: 0
tests               | ===============================================

@ebyhr ebyhr requested a review from wendigo November 4, 2025 01:25
@ebyhr ebyhr merged commit 6efc9eb into trinodb:master Nov 4, 2025
168 of 170 checks passed
@ebyhr ebyhr deleted the ebi/loki-product-test branch November 4, 2025 07:06
@github-actions github-actions bot added this to the 479 milestone Nov 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed loki Loki connector

Development

Successfully merging this pull request may close these issues.

Loki connector broken since 477 (okhttp version mixup) Loki catalog is not correctly loaded on EnvMultinodeAllConnectors

2 participants