Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OPIK-493] Fix Redis setup to properly config database indexes #779

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.comet.opik.infrastructure;

import com.comet.opik.infrastructure.redis.RedisUrlParser;
import com.comet.opik.utils.JsonUtils;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.validation.Valid;
Expand All @@ -19,10 +20,13 @@ public class RedisConfig {
public Config build() {
Config config = new Config();

var redisUrl = RedisUrlParser.parse(singleNodeUrl);
thiagohora marked this conversation as resolved.
Show resolved Hide resolved

Objects.requireNonNull(singleNodeUrl, "singleNodeUrl must not be null");

config.useSingleServer()
.setAddress(singleNodeUrl);
.setAddress(singleNodeUrl)
.setDatabase(redisUrl.database());

config.setCodec(new JsonJacksonCodec(JsonUtils.MAPPER));
return config;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package com.comet.opik.infrastructure.redis;

import java.net.URI;
import java.net.URISyntaxException;

public record RedisUrlParser(String scheme, String host, int port, int database) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need all these params? They are never used apart from tests. Only database is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because a dto with only the database index is not reusable


public static RedisUrlParser parse(String redisUrl) {
try {
URI uri = new URI(redisUrl);
String scheme = uri.getScheme();
String host = uri.getHost();
int port = uri.getPort() == -1 ? 6379 : uri.getPort();
String path = uri.getPath();
int database = 0;

if (path != null && path.length() > 1) {
database = getDatabase(path);
}

return new RedisUrlParser(scheme, host, port, database);
} catch (URISyntaxException e) {
throw new IllegalArgumentException(e);
}
}

private static int getDatabase(String path) {
try {
return Integer.parseInt(path.substring(1));
} catch (NumberFormatException e) {
throw new IllegalArgumentException("Invalid database index in Redis URL: " + path);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3295,12 +3295,12 @@ void createAndGetCost(Map<String, Integer> usage, String model, JsonNode metadat
createAndAssert(expectedSpan, API_KEY, TEST_WORKSPACE);

BigDecimal expectedCost = ModelPrice.fromString(
StringUtils.isNotBlank(model) ?
model :
Optional.ofNullable(metadata)
StringUtils.isNotBlank(model)
? model
: Optional.ofNullable(metadata)
.map(md -> md.get("model"))
.map(JsonNode::asText).orElse("")
).calculateCost(usage);
.map(JsonNode::asText).orElse(""))
.calculateCost(usage);

Span span = getAndAssert(expectedSpan, API_KEY, TEST_WORKSPACE);

Expand All @@ -3317,9 +3317,11 @@ Stream<Arguments> createAndGetCost() {
"{\"created_from\":\"openai\",\"type\":\"openai_chat\",\"model\":\"gpt-3.5-turbo\"}");
return Stream.of(
Arguments.of(Map.of("completion_tokens", Math.abs(podamFactory.manufacturePojo(Integer.class)),
"prompt_tokens", Math.abs(podamFactory.manufacturePojo(Integer.class))), "gpt-3.5-turbo-1106", null),
"prompt_tokens", Math.abs(podamFactory.manufacturePojo(Integer.class))), "gpt-3.5-turbo-1106",
null),
Arguments.of(Map.of("completion_tokens", Math.abs(podamFactory.manufacturePojo(Integer.class)),
"prompt_tokens", Math.abs(podamFactory.manufacturePojo(Integer.class))), "gpt-3.5-turbo-1106", metadata),
"prompt_tokens", Math.abs(podamFactory.manufacturePojo(Integer.class))), "gpt-3.5-turbo-1106",
metadata),
Arguments.of(Map.of("completion_tokens", Math.abs(podamFactory.manufacturePojo(Integer.class)),
"prompt_tokens", Math.abs(podamFactory.manufacturePojo(Integer.class))), null, metadata),
Arguments.of(null, "gpt-3.5-turbo-1106", null),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package com.comet.opik.infrastructure.redis;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import java.util.stream.Stream;

import static org.junit.jupiter.api.Assertions.*;
thiagohora marked this conversation as resolved.
Show resolved Hide resolved

@DisplayName("Redis URL parser Unit Test")
class RedisUrlParserTest {

public static Stream<Arguments> testRedisUrlParser() {
return Stream.of(
Arguments.of("redis://localhost:6379/0", "redis", "localhost", 6379, 0),
Arguments.of("redis://localhost:6379/1", "redis", "localhost", 6379, 1),
Arguments.of("redis://localhost:6379", "redis", "localhost", 6379, 0),
Arguments.of("rediss://localhost:6379", "rediss", "localhost", 6379, 0),
Arguments.of("rediss://master.redis.cache.amazonaws.com:7000/3", "rediss",
"master.redis.cache.amazonaws.com", 7000, 3));
}

@ParameterizedTest
@MethodSource
@DisplayName("Test parse method with different URLs")
void testRedisUrlParser(String redisUrl, String scheme, String host, int port, int database) {
RedisUrlParser redisUrlParser = RedisUrlParser.parse(redisUrl);

assertEquals(scheme, redisUrlParser.scheme());
thiagohora marked this conversation as resolved.
Show resolved Hide resolved
assertEquals(host, redisUrlParser.host());
assertEquals(port, redisUrlParser.port());
assertEquals(database, redisUrlParser.database());
}

}
thiagohora marked this conversation as resolved.
Show resolved Hide resolved