From 51e590284ff53a3f1b94fe6ca410aa4a041b7811 Mon Sep 17 00:00:00 2001 From: Fabian Richter Date: Fri, 25 Oct 2024 13:36:20 +0200 Subject: [PATCH 1/4] feat(server): allow A/B test with a restricted set of rules --- .../org/languagetool/server/TextChecker.java | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java b/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java index 1a6a9e06e04d..8074e2d901c1 100644 --- a/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java +++ b/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java @@ -104,6 +104,34 @@ protected abstract DetectedLanguage getLanguage(String text, Map private long pingsCleanDateMillis = System.currentTimeMillis(); PipelinePool pipelinePool; // mocked in test -> package-private / not final + private final static List onlyTestUsers; + private final static List onlyTestRules; + private final static List onlyTestLanguages; + private final static List onlyTestClients; + + static { + String onlyUsersEnv = System.getenv("LT_TEST_ONLY_USERS"); + if (onlyUsersEnv == null) { + onlyUsersEnv = ""; + } + String onlyRulesEnv = System.getenv("LT_TEST_ONLY_RULES"); + if (onlyRulesEnv == null) { + onlyRulesEnv = ""; + } + String onlyLanguagesEnv = System.getenv("LT_TEST_ONLY_LANGUAGES"); + if (onlyLanguagesEnv == null) { + onlyLanguagesEnv = ""; + } + String onlyClientsEnv = System.getenv("LT_TEST_ONLY_CLIENT"); + if (onlyClientsEnv == null) { + onlyClientsEnv = ""; + } + onlyTestUsers = Arrays.asList(onlyUsersEnv.split(",")); + onlyTestRules = Arrays.asList(onlyRulesEnv.split(",")); + onlyTestLanguages = Arrays.asList(onlyLanguagesEnv.split(",")); + onlyTestClients = Arrays.asList(onlyClientsEnv.split(",")); + } + TextChecker(HTTPServerConfig config, boolean internalServer, Queue workQueue, RequestCounter reqCounter) { this.config = config; this.workQueue = workQueue; @@ -464,6 +492,14 @@ public RuleMatch[] match(AnalyzedSentence sentence) throws IOException { } List enabledRules = getEnabledRuleIds(params); + if ((onlyTestUsers.contains(params.getOrDefault("username", "")) || + (abTest != null && abTest.contains("only"))) && + onlyTestLanguages.contains(lang.getShortCodeWithCountryAndVariant()) && + onlyTestClients.contains(agent)) { + useEnabledOnly = true; + enabledRules = onlyTestRules; + } + List disabledRules = getDisabledRuleIds(params); List enabledCategories = getCategoryIds("enabledCategories", params); List disabledCategories = getCategoryIds("disabledCategories", params); From f059ec48904984e0eb445742a94f2de9f576758f Mon Sep 17 00:00:00 2001 From: Fabian Richter Date: Fri, 25 Oct 2024 15:35:27 +0200 Subject: [PATCH 2/4] Improve parsing of variables for A/B test Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- .../org/languagetool/server/TextChecker.java | 71 +++++++++++++++---- 1 file changed, 58 insertions(+), 13 deletions(-) diff --git a/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java b/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java index 8074e2d901c1..254d6a709ec6 100644 --- a/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java +++ b/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java @@ -104,32 +104,77 @@ protected abstract DetectedLanguage getLanguage(String text, Map private long pingsCleanDateMillis = System.currentTimeMillis(); PipelinePool pipelinePool; // mocked in test -> package-private / not final + /** + * List of usernames for A/B testing with restricted rules. + * Populated from LT_TEST_ONLY_USERS environment variable. + * Format: comma-separated list of usernames + */ private final static List onlyTestUsers; + + /** + * List of rule IDs enabled for A/B testing. + * Populated from LT_TEST_ONLY_RULES environment variable. + * Format: comma-separated list of rule IDs + */ private final static List onlyTestRules; + + /** + * List of languages enabled for A/B testing. + * Populated from LT_TEST_ONLY_LANGUAGES environment variable. + * Format: comma-separated list of language codes + */ private final static List onlyTestLanguages; + + /** + * List of clients enabled for A/B testing. + * Populated from LT_TEST_ONLY_CLIENTS environment variable. + * Format: comma-separated list of client identifiers + */ private final static List onlyTestClients; static { String onlyUsersEnv = System.getenv("LT_TEST_ONLY_USERS"); - if (onlyUsersEnv == null) { - onlyUsersEnv = ""; + if (onlyUsersEnv == null || onlyUsersEnv.trim().isEmpty()) { + onlyTestUsers = Collections.emptyList(); + } else { + onlyTestUsers = Arrays.stream(onlyUsersEnv.split(",")) + .map(String::trim) + .filter(s -> !s.isEmpty()) + .collect(Collectors.toList()); } + String onlyRulesEnv = System.getenv("LT_TEST_ONLY_RULES"); - if (onlyRulesEnv == null) { - onlyRulesEnv = ""; + if (onlyRulesEnv == null || onlyRulesEnv.trim().isEmpty()) { + onlyTestRules = Collections.emptyList(); + } else { + onlyTestRules = Arrays.stream(onlyRulesEnv.split(",")) + .map(String::trim) + .filter(s -> !s.isEmpty()) + .collect(Collectors.toList()); } + String onlyLanguagesEnv = System.getenv("LT_TEST_ONLY_LANGUAGES"); - if (onlyLanguagesEnv == null) { - onlyLanguagesEnv = ""; + if (onlyLanguagesEnv == null || onlyLanguagesEnv.trim().isEmpty()) { + onlyTestLanguages = Collections.emptyList(); + } else { + onlyTestLanguages = Arrays.stream(onlyLanguagesEnv.split(",")) + .map(String::trim) + .filter(s -> !s.isEmpty()) + .collect(Collectors.toList()); } - String onlyClientsEnv = System.getenv("LT_TEST_ONLY_CLIENT"); - if (onlyClientsEnv == null) { - onlyClientsEnv = ""; + + String onlyClientsEnv = System.getenv("LT_TEST_ONLY_CLIENTS"); + if (onlyClientsEnv == null || onlyClientsEnv.trim().isEmpty()) { + onlyTestClients = Collections.emptyList(); + } else { + onlyTestClients = Arrays.stream(onlyClientsEnv.split(",")) + .map(String::trim) + .filter(s -> !s.isEmpty()) + .collect(Collectors.toList()); } - onlyTestUsers = Arrays.asList(onlyUsersEnv.split(",")); - onlyTestRules = Arrays.asList(onlyRulesEnv.split(",")); - onlyTestLanguages = Arrays.asList(onlyLanguagesEnv.split(",")); - onlyTestClients = Arrays.asList(onlyClientsEnv.split(",")); + + log.info("Initialized A/B test restrictions - users: {}, rules: {}, languages: {}, clients: {}", + onlyTestUsers, onlyTestRules, onlyTestLanguages, onlyTestClients); } TextChecker(HTTPServerConfig config, boolean internalServer, Queue workQueue, RequestCounter reqCounter) { From 8ab8678624f8e0d11ea3d9f2aef504050f2de02e Mon Sep 17 00:00:00 2001 From: Fabian Richter Date: Fri, 25 Oct 2024 15:37:15 +0200 Subject: [PATCH 3/4] refactor(server): clean up code for restricted rules test Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- .../java/org/languagetool/server/TextChecker.java | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java b/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java index 254d6a709ec6..9ac7001674ae 100644 --- a/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java +++ b/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java @@ -537,10 +537,16 @@ public RuleMatch[] match(AnalyzedSentence sentence) throws IOException { } List enabledRules = getEnabledRuleIds(params); - if ((onlyTestUsers.contains(params.getOrDefault("username", "")) || - (abTest != null && abTest.contains("only"))) && - onlyTestLanguages.contains(lang.getShortCodeWithCountryAndVariant()) && - onlyTestClients.contains(agent)) { + private boolean shouldApplyTestRules(Map params, String agent, Language lang, List abTest) { + String username = params.getOrDefault("username", ""); + return (onlyTestUsers.contains(username) || (abTest != null && abTest.contains("only"))) && + onlyTestLanguages.contains(lang.getShortCodeWithCountryAndVariant()) && + onlyTestClients.contains(agent); + } + + if (shouldApplyTestRules(params, agent, lang, abTest)) { + log.debug("Applying test rules for user: {}, language: {}, client: {}", + params.getOrDefault("username", ""), lang.getShortCodeWithCountryAndVariant(), agent); useEnabledOnly = true; enabledRules = onlyTestRules; } From 14a51629f7c3f74e23261dfab491fb1f647508a5 Mon Sep 17 00:00:00 2001 From: Fabian Richter Date: Fri, 25 Oct 2024 15:42:58 +0200 Subject: [PATCH 4/4] fix: syntax errors after applying suggestions --- .../org/languagetool/server/TextChecker.java | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java b/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java index 9ac7001674ae..c66b033addf3 100644 --- a/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java +++ b/languagetool-server/src/main/java/org/languagetool/server/TextChecker.java @@ -112,21 +112,21 @@ protected abstract DetectedLanguage getLanguage(String text, Map private final static List onlyTestUsers; /** - * List of rule IDs enabled for A/B testing. + * List of rule IDs enabled for restricted rules A/B testing. * Populated from LT_TEST_ONLY_RULES environment variable. * Format: comma-separated list of rule IDs */ private final static List onlyTestRules; /** - * List of languages enabled for A/B testing. + * List of languages enabled for restricted rules A/B testing. * Populated from LT_TEST_ONLY_LANGUAGES environment variable. * Format: comma-separated list of language codes */ private final static List onlyTestLanguages; /** - * List of clients enabled for A/B testing. + * List of clients enabled for restricted rules A/B testing. * Populated from LT_TEST_ONLY_CLIENTS environment variable. * Format: comma-separated list of client identifiers */ @@ -173,8 +173,10 @@ protected abstract DetectedLanguage getLanguage(String text, Map .collect(Collectors.toList()); } - log.info("Initialized A/B test restrictions - users: {}, rules: {}, languages: {}, clients: {}", - onlyTestUsers, onlyTestRules, onlyTestLanguages, onlyTestClients); + if (!onlyTestRules.isEmpty()) { + log.info("Initialized A/B test restrictions - users: {}, rules: {}, languages: {}, clients: {}", + onlyTestUsers, onlyTestRules, onlyTestLanguages, onlyTestClients); + } } TextChecker(HTTPServerConfig config, boolean internalServer, Queue workQueue, RequestCounter reqCounter) { @@ -322,6 +324,13 @@ void shutdownNow() { RemoteRule.shutdown(); } + private boolean shouldRunRestrictedRulesTest(Map params, String agent, Language lang, List abTest) { + String username = params.getOrDefault("username", ""); + return (onlyTestUsers.contains(username) || (abTest != null && abTest.contains("only"))) && + onlyTestLanguages.contains(lang.getShortCodeWithCountryAndVariant()) && + onlyTestClients.contains(agent); + } + void checkText(AnnotatedText aText, HttpExchange httpExchange, Map params, ErrorRequestLimiter errorRequestLimiter, String remoteAddress) throws Exception { checkParams(params); @@ -537,15 +546,9 @@ public RuleMatch[] match(AnalyzedSentence sentence) throws IOException { } List enabledRules = getEnabledRuleIds(params); - private boolean shouldApplyTestRules(Map params, String agent, Language lang, List abTest) { - String username = params.getOrDefault("username", ""); - return (onlyTestUsers.contains(username) || (abTest != null && abTest.contains("only"))) && - onlyTestLanguages.contains(lang.getShortCodeWithCountryAndVariant()) && - onlyTestClients.contains(agent); - } - if (shouldApplyTestRules(params, agent, lang, abTest)) { - log.debug("Applying test rules for user: {}, language: {}, client: {}", + if (shouldRunRestrictedRulesTest(params, agent, lang, abTest)) { + log.info("Running test with restricted rules for user: {}, language: {}, client: {}", params.getOrDefault("username", ""), lang.getShortCodeWithCountryAndVariant(), agent); useEnabledOnly = true; enabledRules = onlyTestRules;