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-595]: Add Redis Caching part 1 #1015

Merged
merged 13 commits into from
Jan 14, 2025
13 changes: 13 additions & 0 deletions apps/opik-backend/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -243,3 +243,16 @@ llmProviderClient:
# Default: 2023-06-01
# Description: Anthropic API version https://docs.anthropic.com/en/api/versioning
version: ${LLM_PROVIDER_ANTHROPIC_VERSION:-'2023-06-01'}

# Configuration for cache manager
cacheManager:
# Default: true
# Description: Whether or not cache manager is enabled
enabled: ${CACHE_MANAGER_ENABLED:-true}
# Default: PT1S
# Description: Time to live for cache entries in using the formats accepted are based on the ISO-8601: https://docs.oracle.com/javase/8/docs/api/java/time/Duration.html#parse-java.lang.CharSequence-
defaultDuration: ${CACHE_MANAGER_DEFAULT_DURATION:-PT1S}
caches:
# Default: {}
# Description: Dynamically created caches with their respective time to live in seconds
automationRules: ${CACHE_MANAGER_AUTOMATION_RULES_DURATION:-PT1S}
10 changes: 9 additions & 1 deletion apps/opik-backend/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,11 @@
<artifactId>httpclient5</artifactId>
<version>5.2.3</version>
</dependency>
<dependency>
<groupId>org.mvel</groupId>
<artifactId>mvel2</artifactId>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor: we could evaluate using Drools as well. But this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you believe Drools, it's better that we use it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be ok to go with this as well.

<version>2.5.2.Final</version>
</dependency>
<dependency>
<groupId>org.mapstruct</groupId>
<artifactId>mapstruct</artifactId>
Expand Down Expand Up @@ -286,7 +291,7 @@
<dependency>
<groupId>com.clickhouse</groupId>
<artifactId>clickhouse-jdbc</artifactId>
<version>0.7.0</version>
<version>${clickhouse-java.version}</version>
<scope>test</scope>
</dependency>
<dependency>
Expand Down Expand Up @@ -442,6 +447,9 @@
<configuration>
<source>21</source>
<target>21</target>
<compilerArgument>-parameters</compilerArgument>
<parameters>true</parameters>
<testCompilerArgument>-parameters</testCompilerArgument>
<annotationProcessorPaths>
<path>
<groupId>org.projectlombok</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.comet.opik.infrastructure.bi.BiModule;
import com.comet.opik.infrastructure.bi.OpikGuiceyLifecycleEventListener;
import com.comet.opik.infrastructure.bundle.LiquibaseBundle;
import com.comet.opik.infrastructure.cache.CacheModule;
import com.comet.opik.infrastructure.db.DatabaseAnalyticsModule;
import com.comet.opik.infrastructure.db.IdGeneratorModule;
import com.comet.opik.infrastructure.db.NameGeneratorModule;
Expand Down Expand Up @@ -72,7 +73,7 @@ public void initialize(Bootstrap<OpikConfiguration> bootstrap) {
.withPlugins(new SqlObjectPlugin(), new Jackson2Plugin()))
.modules(new DatabaseAnalyticsModule(), new IdGeneratorModule(), new AuthModule(), new RedisModule(),
new RateLimitModule(), new NameGeneratorModule(), new HttpModule(), new EventModule(),
new ConfigurationModule(), new BiModule())
new ConfigurationModule(), new BiModule(), new CacheModule())
.installers(JobGuiceyInstaller.class)
.listen(new OpikGuiceyLifecycleEventListener())
.enableAutoConfig()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public Response getEvaluator(@PathParam("projectId") UUID projectId, @PathParam(
String workspaceId = requestContext.get().getWorkspaceId();

log.info("Looking for automated evaluator: id '{}' on project_id '{}'", projectId, workspaceId);
AutomationRuleEvaluator evaluator = service.findById(evaluatorId, projectId, workspaceId);
AutomationRuleEvaluator<?> evaluator = service.findById(evaluatorId, projectId, workspaceId);
log.info("Found automated evaluator: id '{}' on project_id '{}'", projectId, workspaceId);

return Response.ok().entity(evaluator).build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ public sealed interface AutomationRuleEvaluatorModel<T> extends AutomationRuleMo

@Json
T code();

AutomationRuleEvaluatorType type();

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.Set;
import java.util.UUID;

import static com.comet.opik.api.AutomationRuleEvaluator.AutomationRuleEvaluatorPage;
import static com.comet.opik.infrastructure.db.TransactionTemplateAsync.READ_ONLY;
import static com.comet.opik.infrastructure.db.TransactionTemplateAsync.WRITE;

Expand All @@ -42,14 +43,14 @@ <E, T extends AutomationRuleEvaluator<E>> T findById(@NonNull UUID id, @NonNull

void delete(@NonNull Set<UUID> ids, @NonNull UUID projectId, @NonNull String workspaceId);

AutomationRuleEvaluator.AutomationRuleEvaluatorPage find(@NonNull UUID projectId, @NonNull String workspaceId,
AutomationRuleEvaluatorPage find(@NonNull UUID projectId, @NonNull String workspaceId,
String name, int page, int size);

List<AutomationRuleEvaluatorLlmAsJudge> findAll(@NonNull UUID projectId, @NonNull String workspaceId,
AutomationRuleEvaluatorType automationRuleEvaluatorType);
}

@NonNull @Singleton
@Singleton
@RequiredArgsConstructor(onConstructor_ = @Inject)
@Slf4j
class AutomationRuleEvaluatorServiceImpl implements AutomationRuleEvaluatorService {
Expand All @@ -58,10 +59,9 @@ class AutomationRuleEvaluatorServiceImpl implements AutomationRuleEvaluatorServi

private final @NonNull IdGenerator idGenerator;
private final @NonNull TransactionTemplate template;
private final int DEFAULT_PAGE_LIMIT = 10;

@Override
public <E, T extends AutomationRuleEvaluator<E>> T save(T inputRuleEvaluator,
public <E, T extends AutomationRuleEvaluator<E>> T save(@NonNull T inputRuleEvaluator,
@NonNull String workspaceId,
@NonNull String userName) {

Expand Down Expand Up @@ -146,7 +146,7 @@ public <E, T extends AutomationRuleEvaluator<E>> T findById(@NonNull UUID id, @N
log.debug("Finding AutomationRuleEvaluator with id '{}' in projectId '{}' and workspaceId '{}'", id, projectId,
workspaceId);

return (T) template.inTransaction(READ_ONLY, handle -> {
return template.inTransaction(READ_ONLY, handle -> {
var dao = handle.attach(AutomationRuleEvaluatorDAO.class);
var singleIdSet = Collections.singleton(id);
var criteria = AutomationRuleEvaluatorCriteria.builder().ids(singleIdSet).build();
Expand All @@ -157,6 +157,7 @@ public <E, T extends AutomationRuleEvaluator<E>> T findById(@NonNull UUID id, @N
case LlmAsJudgeAutomationRuleEvaluatorModel llmAsJudge ->
AutomationModelEvaluatorMapper.INSTANCE.map(llmAsJudge);
})
.map(evaluator -> (T) evaluator)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor: for a safer cast, better check if instance of in a filter. Anyhow, it might not ever happen or maybe it's ok to allow this error to bubble up if it ever happens an unexpected programming error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, given the switch, I imagine this would not happen, but Sonar keeps complaining. I changed it so it only complains in one line instead of the whole method

.orElseThrow(this::newNotFoundException);
});
}
Expand Down Expand Up @@ -187,10 +188,8 @@ private NotFoundException newNotFoundException() {
}

@Override
public AutomationRuleEvaluator.AutomationRuleEvaluatorPage find(@NonNull UUID projectId,
@NonNull String workspaceId,
String name,
int pageNum, int size) {
public AutomationRuleEvaluatorPage find(@NonNull UUID projectId, @NonNull String workspaceId,
String name, int pageNum, int size) {

log.debug("Finding AutomationRuleEvaluators with name pattern '{}' in projectId '{}' and workspaceId '{}'",
name, projectId, workspaceId);
Expand All @@ -210,10 +209,9 @@ public AutomationRuleEvaluator.AutomationRuleEvaluatorPage find(@NonNull UUID pr
.toList();
log.info("Found {} AutomationRuleEvaluators for projectId '{}'", automationRuleEvaluators.size(),
projectId);
return new AutomationRuleEvaluator.AutomationRuleEvaluatorPage(pageNum, automationRuleEvaluators.size(),
total,
automationRuleEvaluators);

return new AutomationRuleEvaluatorPage(pageNum, automationRuleEvaluators.size(), total,
automationRuleEvaluators);
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,12 @@ class LlmProviderApiKeyServiceImpl implements LlmProviderApiKeyService {
@Override
public ProviderApiKey find(@NonNull UUID id, @NonNull String workspaceId) {

ProviderApiKey providerApiKey = template.inTransaction(READ_ONLY, handle -> {
return template.inTransaction(READ_ONLY, handle -> {

var repository = handle.attach(LlmProviderApiKeyDAO.class);

return repository.fetch(id, workspaceId).orElseThrow(this::createNotFoundError);
});

return providerApiKey;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package com.comet.opik.infrastructure;

import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.validation.Valid;
import jakarta.validation.constraints.NotNull;
import lombok.Data;

import java.time.Duration;
import java.util.Map;
import java.util.Optional;

@Data
public class CacheConfiguration {

@Valid
@JsonProperty
private boolean enabled = false;

@Valid
@JsonProperty
@NotNull private Duration defaultDuration;

@Valid
@JsonProperty
private Map<String, Duration> caches;

public Map<String, Duration> getCaches() {
return Optional.ofNullable(caches).orElse(Map.of());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,8 @@ public class OpikConfiguration extends JobConfiguration {
@Valid
@NotNull @JsonProperty
private LlmProviderClientConfig llmProviderClient = new LlmProviderClientConfig();

@Valid
@NotNull @JsonProperty
private CacheConfiguration cacheManager = new CacheConfiguration();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package com.comet.opik.infrastructure.cache;

import java.lang.annotation.Documented;
import java.lang.annotation.ElementType;
import java.lang.annotation.Inherited;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

@Target({ElementType.METHOD})
@Retention(RetentionPolicy.RUNTIME)
@Inherited
@Documented
public @interface CacheEvict {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We discussed the JCache approach, but what about just reusing their interface annotations. Any blocker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The JCache annotations only have a cache name property and other configuration classes, such as a cache resolver and a key generator (which must be implemented using reflection based on the method signature). For those reasons, I kept these annotations, which have a cache name and key. We use MVEL to solve the key expression. I believe this is less verbose and a bit more flexible.


/**
* @return the name of the cache group.
* */
String name();

/**
* key is a SpEL expression implemented using MVEL. Please refer to the <a href="http://mvel.documentnode.com/">MVEL documentation for more information</a>.
*
* @return SpEL expression evaluated to generate the cache key.
* */
String key();
}
Loading
Loading