From 7f3116c8786027efa5db8aae3ac25f8993210793 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Mon, 15 Apr 2024 09:26:50 -0400 Subject: [PATCH] fix(recordingOptions): scope customizers to each Target (#341) --- .../cryostat/recordings/RecordingHelper.java | 2 +- .../RecordingOptionsBuilderFactory.java | 26 ++++++++-- .../RecordingOptionsCustomizerFactory.java | 48 +++++++++++++++++++ .../io/cryostat/recordings/Recordings.java | 45 +++++++++-------- .../cryostat/recordings/RecordingsModule.java | 20 -------- .../java/io/cryostat/rules/RuleService.java | 9 ++-- 6 files changed, 96 insertions(+), 54 deletions(-) create mode 100644 src/main/java/io/cryostat/recordings/RecordingOptionsCustomizerFactory.java diff --git a/src/main/java/io/cryostat/recordings/RecordingHelper.java b/src/main/java/io/cryostat/recordings/RecordingHelper.java index 8ba809f86..506febebb 100644 --- a/src/main/java/io/cryostat/recordings/RecordingHelper.java +++ b/src/main/java/io/cryostat/recordings/RecordingHelper.java @@ -246,7 +246,7 @@ public ActiveRecording createSnapshot(Target target, JFRConnection connection) String rename = String.format("%s-%d", desc.getName().toLowerCase(), desc.getId()); RecordingOptionsBuilder recordingOptionsBuilder = - recordingOptionsBuilderFactory.create(connection.getService()); + recordingOptionsBuilderFactory.create(target); recordingOptionsBuilder.name(rename); connection.getService().updateRecordingOptions(desc, recordingOptionsBuilder.build()); diff --git a/src/main/java/io/cryostat/recordings/RecordingOptionsBuilderFactory.java b/src/main/java/io/cryostat/recordings/RecordingOptionsBuilderFactory.java index 60f7215c9..9b0dde250 100644 --- a/src/main/java/io/cryostat/recordings/RecordingOptionsBuilderFactory.java +++ b/src/main/java/io/cryostat/recordings/RecordingOptionsBuilderFactory.java @@ -17,9 +17,27 @@ import org.openjdk.jmc.common.unit.QuantityConversionException; import org.openjdk.jmc.flightrecorder.configuration.recording.RecordingOptionsBuilder; -import org.openjdk.jmc.rjmx.services.jfr.IFlightRecorderService; -public interface RecordingOptionsBuilderFactory { - RecordingOptionsBuilder create(IFlightRecorderService service) - throws QuantityConversionException; +import io.cryostat.targets.Target; +import io.cryostat.targets.TargetConnectionManager; + +import io.smallrye.common.annotation.Blocking; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; + +@ApplicationScoped +public class RecordingOptionsBuilderFactory { + + @Inject RecordingOptionsCustomizerFactory customizerFactory; + @Inject TargetConnectionManager connectionManager; + + @Blocking + public RecordingOptionsBuilder create(Target target) throws QuantityConversionException { + return connectionManager.executeConnectedTask( + target, + conn -> + customizerFactory + .create(target) + .apply(new RecordingOptionsBuilder(conn.getService()))); + } } diff --git a/src/main/java/io/cryostat/recordings/RecordingOptionsCustomizerFactory.java b/src/main/java/io/cryostat/recordings/RecordingOptionsCustomizerFactory.java new file mode 100644 index 000000000..9177bf18e --- /dev/null +++ b/src/main/java/io/cryostat/recordings/RecordingOptionsCustomizerFactory.java @@ -0,0 +1,48 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat.recordings; + +import java.util.HashMap; + +import io.cryostat.core.RecordingOptionsCustomizer; +import io.cryostat.targets.Target; +import io.cryostat.targets.Target.EventKind; +import io.cryostat.targets.Target.TargetDiscovery; + +import io.quarkus.vertx.ConsumeEvent; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import org.jboss.logging.Logger; + +@ApplicationScoped +public class RecordingOptionsCustomizerFactory { + + @Inject Logger logger; + + private final HashMap customizers = new HashMap<>(); + + @ConsumeEvent(Target.TARGET_JVM_DISCOVERY) + void onMessage(TargetDiscovery event) { + if (EventKind.LOST.equals(event.kind())) { + customizers.remove(event.serviceRef()); + } + } + + public RecordingOptionsCustomizer create(Target target) { + return customizers.computeIfAbsent( + target, (t) -> new RecordingOptionsCustomizer(logger::debug)); + } +} diff --git a/src/main/java/io/cryostat/recordings/Recordings.java b/src/main/java/io/cryostat/recordings/Recordings.java index 0bcea5e3c..8eea2e436 100644 --- a/src/main/java/io/cryostat/recordings/Recordings.java +++ b/src/main/java/io/cryostat/recordings/Recordings.java @@ -113,7 +113,7 @@ public class Recordings { @Inject TargetConnectionManager connectionManager; @Inject EventBus bus; @Inject RecordingOptionsBuilderFactory recordingOptionsBuilderFactory; - @Inject RecordingOptionsCustomizer recordingOptionsCustomizer; + @Inject RecordingOptionsCustomizerFactory recordingOptionsCustomizerFactory; @Inject EventOptionsBuilder.Factory eventOptionsBuilderFactory; @Inject Clock clock; @Inject S3Client storage; @@ -604,7 +604,7 @@ public Response createRecording( connection -> { RecordingOptionsBuilder optionsBuilder = recordingOptionsBuilderFactory - .create(connection.getService()) + .create(target) .name(recordingName); if (duration.isPresent()) { optionsBuilder.duration(TimeUnit.SECONDS.toMillis(duration.get())); @@ -884,8 +884,7 @@ public Map getRecordingOptions(@RestPath long id) throws Excepti return connectionManager.executeConnectedTask( target, connection -> { - RecordingOptionsBuilder builder = - recordingOptionsBuilderFactory.create(connection.getService()); + RecordingOptionsBuilder builder = recordingOptionsBuilderFactory.create(target); return getRecordingOptions(connection.getService(), builder); }); } @@ -906,6 +905,9 @@ public Response patchRecordingOptionsV1(@RestPath URI connectUrl) { @Blocking @Path("/api/v3/targets/{id}/recordingOptions") @RolesAllowed("read") + @SuppressFBWarnings( + value = "UC_USELESS_OBJECT", + justification = "SpotBugs thinks the options map is unused, but it is used") public Map patchRecordingOptions( @RestPath long id, @RestForm String toDisk, @@ -914,20 +916,20 @@ public Map patchRecordingOptions( throws Exception { final String unsetKeyword = "unset"; - Map form = new HashMap<>(); + Map options = new HashMap<>(); Pattern bool = Pattern.compile("true|false|" + unsetKeyword); if (toDisk != null) { Matcher m = bool.matcher(toDisk); if (!m.matches()) { throw new BadRequestException("Invalid options"); } - form.put("toDisk", toDisk); + options.put("toDisk", toDisk); } if (maxAge != null) { if (!unsetKeyword.equals(maxAge)) { try { Long.parseLong(maxAge); - form.put("maxAge", maxAge); + options.put("maxAge", maxAge); } catch (NumberFormatException e) { throw new BadRequestException("Invalid options"); } @@ -937,31 +939,28 @@ public Map patchRecordingOptions( if (!unsetKeyword.equals(maxSize)) { try { Long.parseLong(maxSize); - form.put("maxSize", maxSize); + options.put("maxSize", maxSize); } catch (NumberFormatException e) { throw new BadRequestException("Invalid options"); } } } - form.entrySet() - .forEach( - e -> { - RecordingOptionsCustomizer.OptionKey optionKey = - RecordingOptionsCustomizer.OptionKey.fromOptionName(e.getKey()) - .get(); - if ("unset".equals(e.getValue())) { - recordingOptionsCustomizer.unset(optionKey); - } else { - recordingOptionsCustomizer.set(optionKey, e.getValue()); - } - }); - Target target = Target.find("id", id).singleResult(); + for (var entry : options.entrySet()) { + RecordingOptionsCustomizer.OptionKey optionKey = + RecordingOptionsCustomizer.OptionKey.fromOptionName(entry.getKey()).get(); + var recordingOptionsCustomizer = recordingOptionsCustomizerFactory.create(target); + if (unsetKeyword.equals(entry.getValue())) { + recordingOptionsCustomizer.unset(optionKey); + } else { + recordingOptionsCustomizer.set(optionKey, entry.getValue()); + } + } + return connectionManager.executeConnectedTask( target, connection -> { - RecordingOptionsBuilder builder = - recordingOptionsBuilderFactory.create(connection.getService()); + var builder = recordingOptionsBuilderFactory.create(target); return getRecordingOptions(connection.getService(), builder); }); } diff --git a/src/main/java/io/cryostat/recordings/RecordingsModule.java b/src/main/java/io/cryostat/recordings/RecordingsModule.java index fa83a5a36..9f04be17c 100644 --- a/src/main/java/io/cryostat/recordings/RecordingsModule.java +++ b/src/main/java/io/cryostat/recordings/RecordingsModule.java @@ -15,37 +15,17 @@ */ package io.cryostat.recordings; -import org.openjdk.jmc.flightrecorder.configuration.recording.RecordingOptionsBuilder; - import io.cryostat.core.EventOptionsBuilder; -import io.cryostat.core.RecordingOptionsCustomizer; import jakarta.enterprise.context.ApplicationScoped; import jakarta.enterprise.inject.Produces; import jakarta.inject.Singleton; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; @Singleton public class RecordingsModule { - - @Produces - @ApplicationScoped - public RecordingOptionsBuilderFactory provideRecordingOptionsBuilderFactory( - RecordingOptionsCustomizer customizer) { - return service -> customizer.apply(new RecordingOptionsBuilder(service)); - } - @Produces @ApplicationScoped public EventOptionsBuilder.Factory provideEventOptionsBuilderFactory() { return new EventOptionsBuilder.Factory(); } - - @Produces - @ApplicationScoped - public RecordingOptionsCustomizer provideRecordingOptionsCustomizer() { - Logger log = LoggerFactory.getLogger(RecordingOptionsCustomizer.class); - return new RecordingOptionsCustomizer(log::debug); - } } diff --git a/src/main/java/io/cryostat/rules/RuleService.java b/src/main/java/io/cryostat/rules/RuleService.java index bf726e608..e1a7165d6 100644 --- a/src/main/java/io/cryostat/rules/RuleService.java +++ b/src/main/java/io/cryostat/rules/RuleService.java @@ -32,7 +32,6 @@ import org.openjdk.jmc.rjmx.ConnectionException; import org.openjdk.jmc.rjmx.ServiceNotAvailableException; -import io.cryostat.core.net.JFRConnection; import io.cryostat.core.templates.Template; import io.cryostat.core.templates.TemplateType; import io.cryostat.expressions.MatchExpressionEvaluator; @@ -143,7 +142,7 @@ public void activate(Rule rule, Target target) throws Exception { connectionManager.executeConnectedTask( target, connection -> { - var recordingOptions = createRecordingOptions(rule, connection); + var recordingOptions = createRecordingOptions(rule, target); Pair pair = recordingHelper.parseEventSpecifier(rule.eventSpecifier); @@ -173,15 +172,13 @@ public void activate(Rule rule, Target target) throws Exception { } } - private IConstrainedMap createRecordingOptions(Rule rule, JFRConnection connection) + private IConstrainedMap createRecordingOptions(Rule rule, Target target) throws ConnectionException, QuantityConversionException, IOException, ServiceNotAvailableException { RecordingOptionsBuilder optionsBuilder = - recordingOptionsBuilderFactory - .create(connection.getService()) - .name(rule.getRecordingName()); + recordingOptionsBuilderFactory.create(target).name(rule.getRecordingName()); if (rule.maxAgeSeconds > 0) { optionsBuilder.maxAge(rule.maxAgeSeconds); }