From 4bdb4a03afde9616e77d3e2b231a7ff2b9599a30 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Tue, 2 May 2023 11:42:31 +0300 Subject: [PATCH 1/2] Skip copying jfr recoding into a temporary file --- profiler/README.md | 8 - .../opentelemetry/profiler/JfrActivator.java | 36 +--- .../opentelemetry/profiler/JfrDirCleanup.java | 69 -------- .../profiler/JfrFileLifecycleEvents.java | 39 ----- .../opentelemetry/profiler/JfrRecorder.java | 53 ++++-- ...hHandler.java => JfrRecordingHandler.java} | 45 ++--- .../profiler/RecordingEscapeHatch.java | 162 ------------------ .../profiler/RecordingSequencer.java | 15 -- .../profiler/util/FileDeleter.java | 46 ----- .../profiler/JfrDirCleanupTest.java | 63 ------- .../profiler/JfrFileLifecycleEventsTest.java | 53 ------ .../profiler/JfrRecorderTest.java | 17 +- .../profiler/RecordingEscapeHatchTest.java | 125 -------------- .../profiler/RecordingSequencerTest.java | 79 +-------- 14 files changed, 60 insertions(+), 750 deletions(-) delete mode 100644 profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrDirCleanup.java delete mode 100644 profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrFileLifecycleEvents.java rename profiler/src/main/java/com/splunk/opentelemetry/profiler/{JfrPathHandler.java => JfrRecordingHandler.java} (71%) delete mode 100644 profiler/src/main/java/com/splunk/opentelemetry/profiler/RecordingEscapeHatch.java delete mode 100644 profiler/src/main/java/com/splunk/opentelemetry/profiler/util/FileDeleter.java delete mode 100644 profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrDirCleanupTest.java delete mode 100644 profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrFileLifecycleEventsTest.java delete mode 100644 profiler/src/test/java/com/splunk/opentelemetry/profiler/RecordingEscapeHatchTest.java diff --git a/profiler/README.md b/profiler/README.md index ac813bde9..243b92ce6 100644 --- a/profiler/README.md +++ b/profiler/README.md @@ -105,14 +105,6 @@ The agent logs the profiling configuration at `INFO` during startup. You can gre [otel.javaagent 2021-09-28 18:17:04:246 +0000] [main] INFO - ----------------------- ``` -### What about this escape hatch? - -If the escape hatch becomes active, it will log with `com.splunk.opentelemetry.profiler.RecordingEscapeHatch` -(you can grep for this in the logs). You may also look for `"** THIS WILL RESULT IN LOSS OF PROFILING DATA **"` -as a big hint that things are not well. - -You may need to free up some disk space and/or give the JVM more resources. - ### What if I'm on an unsupported JVM? If your JVM does not support JFR, the profiler logs a warning at startup with the diff --git a/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrActivator.java b/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrActivator.java index c4760e990..8e830e1b4 100644 --- a/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrActivator.java +++ b/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrActivator.java @@ -20,8 +20,6 @@ import static com.splunk.opentelemetry.profiler.Configuration.CONFIG_KEY_KEEP_FILES; import static com.splunk.opentelemetry.profiler.Configuration.CONFIG_KEY_PROFILER_DIRECTORY; import static com.splunk.opentelemetry.profiler.Configuration.CONFIG_KEY_RECORDING_DURATION; -import static com.splunk.opentelemetry.profiler.JfrFileLifecycleEvents.buildOnFileFinished; -import static com.splunk.opentelemetry.profiler.JfrFileLifecycleEvents.buildOnNewRecording; import static com.splunk.opentelemetry.profiler.util.Runnables.logUncaught; import static java.util.logging.Level.WARNING; @@ -33,7 +31,6 @@ import com.splunk.opentelemetry.profiler.events.EventPeriods; import com.splunk.opentelemetry.profiler.exporter.CpuEventExporter; import com.splunk.opentelemetry.profiler.exporter.PprofCpuEventExporter; -import com.splunk.opentelemetry.profiler.util.FileDeleter; import com.splunk.opentelemetry.profiler.util.HelpfulExecutors; import io.opentelemetry.api.logs.Logger; import io.opentelemetry.javaagent.extension.AgentListener; @@ -52,7 +49,6 @@ import java.time.Duration; import java.util.Map; import java.util.concurrent.ExecutorService; -import java.util.function.Consumer; @AutoService(AgentListener.class) public class JfrActivator implements AgentListener { @@ -127,12 +123,6 @@ private void activateJfrAndRunForever(ConfigProperties config, Resource resource // can't be null, default value is set in Configuration.getProperties Duration recordingDuration = config.getDuration(CONFIG_KEY_RECORDING_DURATION, null); - RecordingEscapeHatch recordingEscapeHatch = - RecordingEscapeHatch.builder() - .namingConvention(namingConvention) - .configKeepsFilesOnDisk(keepFiles(config)) - .recordingDuration(recordingDuration) - .build(); Map jfrSettings = buildJfrSettings(config); EventReader eventReader = new EventReader(); @@ -174,37 +164,27 @@ private void activateJfrAndRunForever(ConfigProperties config, Resource resource EventProcessingChain eventProcessingChain = new EventProcessingChain( eventReader, spanContextualizer, threadDumpProcessor, tlabProcessor); - Consumer deleter = buildFileDeleter(config); - JfrDirCleanup dirCleanup = new JfrDirCleanup(deleter); - Consumer onFileFinished = buildOnFileFinished(deleter, dirCleanup); - - JfrPathHandler jfrPathHandler = - JfrPathHandler.builder() - .eventProcessingChain(eventProcessingChain) - .onFileFinished(onFileFinished) - .build(); - - Consumer onNewRecording = buildOnNewRecording(jfrPathHandler, dirCleanup); + JfrRecordingHandler jfrRecordingHandler = + JfrRecordingHandler.builder().eventProcessingChain(eventProcessingChain).build(); JfrRecorder recorder = JfrRecorder.builder() .settings(jfrSettings) .maxAgeDuration(recordingDuration.multipliedBy(10)) .jfr(JFR.instance) + .onNewRecording(jfrRecordingHandler) .namingConvention(namingConvention) - .onNewRecordingFile(onNewRecording) + .keepRecordingFiles(keepFiles(config)) .build(); RecordingSequencer sequencer = RecordingSequencer.builder() .recordingDuration(recordingDuration) - .recordingEscapeHatch(recordingEscapeHatch) .recorder(recorder) .build(); sequencer.start(); - dirCleanup.registerShutdownHook(); } private Logger buildOtelLogger(LogRecordProcessor logProcessor, Resource resource) { @@ -246,14 +226,6 @@ private Map buildJfrSettings(ConfigProperties config) { return overrides.apply(jfrSettings); } - private Consumer buildFileDeleter(ConfigProperties config) { - if (keepFiles(config)) { - logger.log(WARNING, "{0} is enabled, leaving JFR files on disk.", CONFIG_KEY_KEEP_FILES); - return FileDeleter.noopFileDeleter(); - } - return FileDeleter.newDeleter(); - } - private boolean keepFiles(ConfigProperties config) { return config.getBoolean(CONFIG_KEY_KEEP_FILES, false); } diff --git a/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrDirCleanup.java b/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrDirCleanup.java deleted file mode 100644 index bf783d7a7..000000000 --- a/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrDirCleanup.java +++ /dev/null @@ -1,69 +0,0 @@ -/* - * Copyright Splunk Inc. - * - * 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 com.splunk.opentelemetry.profiler; - -import static com.splunk.opentelemetry.profiler.util.Runnables.logUncaught; -import static java.util.logging.Level.WARNING; - -import com.google.common.annotations.VisibleForTesting; -import java.nio.file.Path; -import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; -import java.util.function.Consumer; -import java.util.logging.Logger; - -/** - * This class is responsible for cleaning up jfr files that might not have otherwise been cleaned - * up. It can register a VM shutdown hook to remove its list of pending files at shutdown. - */ -class JfrDirCleanup { - - private static final Logger logger = Logger.getLogger(JfrDirCleanup.class.getName()); - private final Consumer fileDeleter; - private final Map pending = new ConcurrentHashMap<>(); - - public JfrDirCleanup(Consumer fileDeleter) { - this.fileDeleter = fileDeleter; - } - - public void recordingCreated(Path path) { - pending.put(path, 0); - } - - public void recordingDeleted(Path path) { - pending.remove(path); - } - - public void cleanUp() { - pending - .keySet() - .forEach( - path -> { - logger.log(WARNING, "Shutdown :: JfrDirCleanup deleting {0}", path); - fileDeleter.accept(path); - }); - } - - public void registerShutdownHook() { - getRuntime().addShutdownHook(new Thread(logUncaught(this::cleanUp))); - } - - @VisibleForTesting - protected Runtime getRuntime() { - return Runtime.getRuntime(); - } -} diff --git a/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrFileLifecycleEvents.java b/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrFileLifecycleEvents.java deleted file mode 100644 index dee4419c8..000000000 --- a/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrFileLifecycleEvents.java +++ /dev/null @@ -1,39 +0,0 @@ -/* - * Copyright Splunk Inc. - * - * 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 com.splunk.opentelemetry.profiler; - -import java.nio.file.Path; -import java.util.function.Consumer; - -class JfrFileLifecycleEvents { - - public static Consumer buildOnNewRecording( - Consumer jfrPathHandler, JfrDirCleanup dirCleanup) { - return path -> { - dirCleanup.recordingCreated(path); - jfrPathHandler.accept(path); - }; - } - - public static Consumer buildOnFileFinished( - Consumer deleter, JfrDirCleanup dirCleanup) { - return path -> { - deleter.accept(path); - dirCleanup.recordingDeleted(path); - }; - } -} diff --git a/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrRecorder.java b/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrRecorder.java index 375117456..24ff15538 100644 --- a/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrRecorder.java +++ b/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrRecorder.java @@ -44,8 +44,9 @@ class JfrRecorder { private final Duration maxAgeDuration; private final JFR jfr; - private final Consumer onNewRecordingFile; + private final Consumer onNewRecording; private final RecordingFileNamingConvention namingConvention; + private final boolean keepRecordingFiles; private volatile Recording recording; private volatile Instant snapshotStart = Instant.now(); @@ -53,8 +54,9 @@ class JfrRecorder { this.settings = requireNonNull(builder.settings); this.maxAgeDuration = requireNonNull(builder.maxAgeDuration); this.jfr = requireNonNull(builder.jfr); - this.onNewRecordingFile = requireNonNull(builder.onNewRecordingFile); + this.onNewRecording = requireNonNull(builder.onNewRecording); this.namingConvention = requireNonNull(builder.namingConvention); + this.keepRecordingFiles = builder.keepRecordingFiles; } public void start() { @@ -75,24 +77,33 @@ Recording newRecording() { public void flushSnapshot() { try (Recording snap = jfr.takeSnapshot()) { - Path path = namingConvention.newOutputPath().toAbsolutePath(); - logger.log(FINE, "Flushing a JFR snapshot: {0}", path); Instant snapshotEnd = snap.getStopTime(); - try (InputStream in = snap.getStream(snapshotStart, snapshotEnd)) { - try (OutputStream out = Files.newOutputStream(path)) { - copy(in, out); - if (logger.isLoggable(FINE)) { - logger.log( - FINE, - "Wrote JFR dump {0} with size {1}", - new Object[] {path, path.toFile().length()}); + Instant start = snapshotStart; + snapshotStart = snapshotEnd; + if (keepRecordingFiles) { + Path path = namingConvention.newOutputPath().toAbsolutePath(); + logger.log(FINE, "Flushing a JFR snapshot: {0}", path); + try (InputStream in = snap.getStream(start, snapshotEnd)) { + try (OutputStream out = Files.newOutputStream(path)) { + copy(in, out); + if (logger.isLoggable(FINE)) { + logger.log( + FINE, + "Wrote JFR dump {0} with size {1}", + new Object[] {path, path.toFile().length()}); + } } } + try (InputStream in = Files.newInputStream(path)) { + onNewRecording.accept(in); + } + } else { + try (InputStream in = snap.getStream(start, snapshotEnd)) { + onNewRecording.accept(in); + } } - snapshotStart = snapshotEnd; - onNewRecordingFile.accept(path); } catch (IOException e) { - logger.log(SEVERE, "Error flushing JFR snapshot data to disk", e); + logger.log(SEVERE, "Error handling JFR recording", e); } } @@ -122,7 +133,8 @@ public static class Builder { private Map settings; private Duration maxAgeDuration; private JFR jfr = JFR.instance; - private Consumer onNewRecordingFile; + private Consumer onNewRecording; + private boolean keepRecordingFiles; public Builder settings(Map settings) { this.settings = settings; @@ -139,8 +151,8 @@ public Builder jfr(JFR jfr) { return this; } - public Builder onNewRecordingFile(Consumer onNewRecordingFile) { - this.onNewRecordingFile = onNewRecordingFile; + public Builder onNewRecording(Consumer onNewRecording) { + this.onNewRecording = onNewRecording; return this; } @@ -149,6 +161,11 @@ public Builder namingConvention(RecordingFileNamingConvention namingConvention) return this; } + public Builder keepRecordingFiles(boolean keepRecordingFiles) { + this.keepRecordingFiles = keepRecordingFiles; + return this; + } + public JfrRecorder build() { return new JfrRecorder(this); } diff --git a/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrPathHandler.java b/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrRecordingHandler.java similarity index 71% rename from profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrPathHandler.java rename to profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrRecordingHandler.java index 056856b51..ca7f5efc8 100644 --- a/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrPathHandler.java +++ b/profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrRecordingHandler.java @@ -19,54 +19,39 @@ import static java.util.logging.Level.FINE; import static java.util.logging.Level.SEVERE; -import java.io.RandomAccessFile; +import java.io.InputStream; import java.lang.reflect.Method; -import java.nio.file.Path; import java.time.Duration; import java.time.Instant; import java.util.Collections; -import java.util.List; import java.util.function.Consumer; import java.util.logging.Logger; import javax.annotation.Nullable; import org.openjdk.jmc.common.item.IItemCollection; import org.openjdk.jmc.flightrecorder.EventCollectionUtil; -import org.openjdk.jmc.flightrecorder.internal.ChunkInfo; import org.openjdk.jmc.flightrecorder.internal.FlightRecordingLoader; import org.openjdk.jmc.flightrecorder.internal.IChunkLoader; import org.openjdk.jmc.flightrecorder.internal.IChunkSupplier; import org.openjdk.jmc.flightrecorder.internal.parser.LoaderContext; /** - * Responsible for processing a single jfr file snapshot. It streams events from the - * RecordedEventStream into the processing chain and, once complete, calls the onFileFinished - * callback. + * Responsible for processing a single jfr recording snapshot. It streams events from the recoding + * into the processing chain. */ -class JfrPathHandler implements Consumer { +class JfrRecordingHandler implements Consumer { - private static final Logger logger = Logger.getLogger(JfrPathHandler.class.getName()); + private static final Logger logger = Logger.getLogger(JfrRecordingHandler.class.getName()); private final EventProcessingChain eventProcessingChain; - private final Consumer onFileFinished; - public JfrPathHandler(Builder builder) { + public JfrRecordingHandler(Builder builder) { this.eventProcessingChain = builder.eventProcessingChain; - this.onFileFinished = builder.onFileFinished; } @Override - public void accept(Path path) { - if (logger.isLoggable(FINE)) { - logger.log( - FINE, - "New jfr file detected: {0} size: {1}", - new Object[] {path, path.toFile().length()}); - } + public void accept(InputStream inputStream) { Instant start = Instant.now(); try { - RandomAccessFile raf = new RandomAccessFile(path.toFile(), "r"); - List allChunks = - FlightRecordingLoader.readChunkInfo(FlightRecordingLoader.createChunkSupplier(raf)); - IChunkSupplier chunkSupplier = FlightRecordingLoader.createChunkSupplier(raf, allChunks); + IChunkSupplier chunkSupplier = FlightRecordingLoader.createChunkSupplier(inputStream); byte[] buffer = new byte[0]; while (true) { @@ -82,15 +67,13 @@ public void accept(Path path) { items.stream().flatMap(iItems -> iItems.stream()).forEach(eventProcessingChain::accept); eventProcessingChain.flushBuffer(); } - - onFileFinished.accept(path); } catch (Exception exception) { logger.log(SEVERE, "Error parsing JFR recording", exception); } finally { Instant end = Instant.now(); long timeElapsed = Duration.between(start, end).toMillis(); if (logger.isLoggable(FINE)) { - logger.log(FINE, "Processed {0} in {1}ms", new Object[] {path, timeElapsed}); + logger.log(FINE, "Processed recording in {1}ms", new Object[] {timeElapsed}); } eventProcessingChain.logEventStats(); } @@ -122,20 +105,14 @@ public static Builder builder() { public static class Builder { private EventProcessingChain eventProcessingChain; - private Consumer onFileFinished; public Builder eventProcessingChain(EventProcessingChain eventProcessingChain) { this.eventProcessingChain = eventProcessingChain; return this; } - public Builder onFileFinished(Consumer onFileFinished) { - this.onFileFinished = onFileFinished; - return this; - } - - public JfrPathHandler build() { - return new JfrPathHandler(this); + public JfrRecordingHandler build() { + return new JfrRecordingHandler(this); } } } diff --git a/profiler/src/main/java/com/splunk/opentelemetry/profiler/RecordingEscapeHatch.java b/profiler/src/main/java/com/splunk/opentelemetry/profiler/RecordingEscapeHatch.java deleted file mode 100644 index edd0a8190..000000000 --- a/profiler/src/main/java/com/splunk/opentelemetry/profiler/RecordingEscapeHatch.java +++ /dev/null @@ -1,162 +0,0 @@ -/* - * Copyright Splunk Inc. - * - * 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 com.splunk.opentelemetry.profiler; - -import static java.util.logging.Level.SEVERE; -import static java.util.logging.Level.WARNING; - -import java.io.IOException; -import java.nio.file.FileStore; -import java.nio.file.Files; -import java.nio.file.Path; -import java.time.Duration; -import java.util.logging.Logger; -import java.util.stream.Stream; - -class RecordingEscapeHatch { - - private static final long MIN_FREE_SPACE_BYTES = 100 * 1024 * 1024; // 100MiB - private static final Logger logger = Logger.getLogger(RecordingEscapeHatch.class.getName()); - private static final Duration MAX_PENDING_DURATION = Duration.ofMinutes(5); - - private final RecordingFileNamingConvention namingConvention; - private final boolean configKeepsFilesOnDisk; - private final long maxFileCount; - private final FilesShim filesShim; - - RecordingEscapeHatch(Builder builder) { - this.namingConvention = builder.namingConvention; - this.configKeepsFilesOnDisk = builder.configKeepsFilesOnDisk; - this.filesShim = builder.filesShim; - this.maxFileCount = MAX_PENDING_DURATION.toMillis() / builder.recordingDuration.toMillis(); - } - - public boolean jfrCanContinue() { - boolean result = freeSpaceIsOk() && notFileBacklogged(); - if (!result) { - logger.warning("** THIS WILL RESULT IN LOSS OF PROFILING DATA **"); - } - return result; - } - - private boolean freeSpaceIsOk() { - try { - Path outputPath = namingConvention.getOutputPath(); - FileStore store = filesShim.getFileStore(outputPath); - long usableSpace = store.getUsableSpace(); - boolean result = usableSpace > MIN_FREE_SPACE_BYTES; - if (!result) { - logger.log( - WARNING, - "** NOT STARTING RECORDING, only {0} bytes free, require {1}", - new Object[] {usableSpace, MIN_FREE_SPACE_BYTES}); - } - return result; - } catch (IOException e) { - logger.log(SEVERE, "Could not check free space, assuming everything is bad", e); - return false; - } - } - - /** - * Backlogged means that there are too many recent jfr files sitting on disk, and we think we are - * falling behind. We will never be backlogged if the user has specified - * -Dsplunk.profiler.keep-files=true - */ - private boolean notFileBacklogged() { - if (configKeepsFilesOnDisk) { - return true; - } - try { - return pendingFileCount() < maxFileCount; - } catch (IOException e) { - logger.warning("Error listing files in output directory, assuming everything is bad"); - return false; - } - } - - /** Returns the number of jfr files in the output directory that match our pattern */ - private long pendingFileCount() throws IOException { - Path outputPath = namingConvention.getOutputPath(); - try (Stream files = filesShim.list(outputPath)) { - return files.filter(filesShim::isRegularFile).filter(namingConvention::matches).count(); - } - } - - static Builder builder() { - return new Builder(); - } - - // To help with testing - interface FilesShim { - FilesShim DEFAULT = - new FilesShim() { - @Override - public FileStore getFileStore(Path path) throws IOException { - return Files.getFileStore(path); - } - - @Override - public Stream list(Path path) throws IOException { - return Files.list(path); - } - - @Override - public boolean isRegularFile(Path path) { - return Files.isRegularFile(path); - } - }; - - FileStore getFileStore(Path path) throws IOException; - - Stream list(Path path) throws IOException; - - boolean isRegularFile(Path path); - } - - static class Builder { - - RecordingFileNamingConvention namingConvention; - boolean configKeepsFilesOnDisk = false; - Duration recordingDuration; - FilesShim filesShim = FilesShim.DEFAULT; - - Builder namingConvention(RecordingFileNamingConvention namingConvention) { - this.namingConvention = namingConvention; - return this; - } - - Builder configKeepsFilesOnDisk(boolean configKeepsFilesOnDisk) { - this.configKeepsFilesOnDisk = configKeepsFilesOnDisk; - return this; - } - - Builder recordingDuration(Duration recordingDuration) { - this.recordingDuration = recordingDuration; - return this; - } - - Builder filesShim(FilesShim shim) { - this.filesShim = shim; - return this; - } - - RecordingEscapeHatch build() { - return new RecordingEscapeHatch(this); - } - } -} diff --git a/profiler/src/main/java/com/splunk/opentelemetry/profiler/RecordingSequencer.java b/profiler/src/main/java/com/splunk/opentelemetry/profiler/RecordingSequencer.java index 0ec92a936..35b99f325 100644 --- a/profiler/src/main/java/com/splunk/opentelemetry/profiler/RecordingSequencer.java +++ b/profiler/src/main/java/com/splunk/opentelemetry/profiler/RecordingSequencer.java @@ -35,12 +35,10 @@ class RecordingSequencer { private final ScheduledExecutorService executor = HelpfulExecutors.newSingleThreadedScheduledExecutor("JFR Recording Sequencer"); private final Duration recordingDuration; - private final RecordingEscapeHatch recordingEscapeHatch; private final JfrRecorder recorder; private RecordingSequencer(Builder builder) { this.recordingDuration = builder.recordingDuration; - this.recordingEscapeHatch = builder.recordingEscapeHatch; this.recorder = builder.recorder; } @@ -53,13 +51,6 @@ public void start() { @VisibleForTesting void handleInterval() { try { - if (!recordingEscapeHatch.jfrCanContinue()) { - logger.warning("JFR recordings cannot proceed."); - if (recorder.isStarted()) { - recorder.stop(); - } - return; - } if (!recorder.isStarted()) { recorder.start(); return; @@ -76,7 +67,6 @@ public static Builder builder() { public static class Builder { private Duration recordingDuration; - private RecordingEscapeHatch recordingEscapeHatch; private JfrRecorder recorder; public Builder recordingDuration(Duration duration) { @@ -84,11 +74,6 @@ public Builder recordingDuration(Duration duration) { return this; } - public Builder recordingEscapeHatch(RecordingEscapeHatch prediate) { - this.recordingEscapeHatch = prediate; - return this; - } - public Builder recorder(JfrRecorder recorder) { this.recorder = recorder; return this; diff --git a/profiler/src/main/java/com/splunk/opentelemetry/profiler/util/FileDeleter.java b/profiler/src/main/java/com/splunk/opentelemetry/profiler/util/FileDeleter.java deleted file mode 100644 index 02986df45..000000000 --- a/profiler/src/main/java/com/splunk/opentelemetry/profiler/util/FileDeleter.java +++ /dev/null @@ -1,46 +0,0 @@ -/* - * Copyright Splunk Inc. - * - * 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 com.splunk.opentelemetry.profiler.util; - -import static java.util.logging.Level.WARNING; - -import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.Path; -import java.util.function.Consumer; -import java.util.logging.Logger; - -public interface FileDeleter extends Consumer { - - Logger logger = Logger.getLogger(FileDeleter.class.getName()); - - static FileDeleter newDeleter() { - return path -> { - try { - Files.delete(path); - } catch (IOException e) { - logger.log(WARNING, "Could not delete: " + path, e); - } - }; - } - - static FileDeleter noopFileDeleter() { - return path -> { - logger.log(WARNING, "Leaving {0} on disk.", path); - }; - } -} diff --git a/profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrDirCleanupTest.java b/profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrDirCleanupTest.java deleted file mode 100644 index 6e0e913d5..000000000 --- a/profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrDirCleanupTest.java +++ /dev/null @@ -1,63 +0,0 @@ -/* - * Copyright Splunk Inc. - * - * 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 com.splunk.opentelemetry.profiler; - -import static org.mockito.Mockito.doNothing; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoMoreInteractions; - -import java.nio.file.Path; -import java.nio.file.Paths; -import java.util.function.Consumer; -import org.junit.jupiter.api.Test; -import org.mockito.ArgumentCaptor; - -class JfrDirCleanupTest { - - @Test - void testLifecycle() { - Path path1 = Paths.get("/some/path/to/file1.jfr"); - Path path2 = Paths.get("/some/path/to/file1.jfr"); - Path path3 = Paths.get("/some/path/to/file1.jfr"); - - Consumer fileDeleter = mock(Consumer.class); - Runtime runtime = mock(Runtime.class); - ArgumentCaptor threadCaptor = ArgumentCaptor.forClass(Thread.class); - doNothing().when(runtime).addShutdownHook(threadCaptor.capture()); - - JfrDirCleanup jfrDirCleanup = - new JfrDirCleanup(fileDeleter) { - @Override - protected Runtime getRuntime() { - return runtime; - } - }; - jfrDirCleanup.registerShutdownHook(); - - jfrDirCleanup.recordingCreated(path1); - jfrDirCleanup.recordingCreated(path2); - jfrDirCleanup.recordingDeleted(path1); - jfrDirCleanup.recordingCreated(path3); - - threadCaptor.getValue().run(); // not start, just runs inline - - verify(fileDeleter).accept(path2); - verify(fileDeleter).accept(path3); - verifyNoMoreInteractions(fileDeleter); - } -} diff --git a/profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrFileLifecycleEventsTest.java b/profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrFileLifecycleEventsTest.java deleted file mode 100644 index da43c747f..000000000 --- a/profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrFileLifecycleEventsTest.java +++ /dev/null @@ -1,53 +0,0 @@ -/* - * Copyright Splunk Inc. - * - * 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 com.splunk.opentelemetry.profiler; - -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; - -import java.nio.file.Path; -import java.nio.file.Paths; -import java.util.function.Consumer; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; - -@ExtendWith(MockitoExtension.class) -class JfrFileLifecycleEventsTest { - - @Mock JfrDirCleanup cleanup; - Path path = Paths.get("/path/to/some/file.jfr"); - - @Test - void testOnNewRecording() { - Consumer pathHandler = mock(Consumer.class); - Consumer callback = JfrFileLifecycleEvents.buildOnNewRecording(pathHandler, cleanup); - callback.accept(path); - verify(pathHandler).accept(path); - verify(cleanup).recordingCreated(path); - } - - @Test - void testOnFileFinished() { - Consumer deleter = mock(Consumer.class); - Consumer callback = JfrFileLifecycleEvents.buildOnFileFinished(deleter, cleanup); - callback.accept(path); - verify(deleter).accept(path); - verify(cleanup).recordingDeleted(path); - } -} diff --git a/profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrRecorderTest.java b/profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrRecorderTest.java index 999f59ff7..70d67d688 100644 --- a/profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrRecorderTest.java +++ b/profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrRecorderTest.java @@ -18,12 +18,13 @@ import static com.splunk.opentelemetry.profiler.JfrRecorder.RECORDING_NAME; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.*; import java.io.ByteArrayInputStream; import java.io.IOException; -import java.nio.file.Path; +import java.io.InputStream; import java.time.Duration; import java.util.HashMap; import java.util.Map; @@ -33,7 +34,6 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.junit.jupiter.api.io.TempDir; import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; @@ -41,11 +41,10 @@ @ExtendWith(MockitoExtension.class) class JfrRecorderTest { - @TempDir Path outDir; Duration maxAge = Duration.ofMinutes(13); Map settings; @Mock Recording recording; - @Mock Consumer onNewRecordingFile; + @Mock Consumer onNewRecording; @BeforeEach void setup() { @@ -69,15 +68,14 @@ void testStart() { void testFlushSnapshot() throws Exception { JFR jfr = mock(JFR.class); Recording snap = mock(Recording.class); - ArgumentCaptor pathCaptor = ArgumentCaptor.forClass(Path.class); - doNothing().when(onNewRecordingFile).accept(pathCaptor.capture()); + ArgumentCaptor inputStreamCaptor = ArgumentCaptor.forClass(InputStream.class); + doNothing().when(onNewRecording).accept(inputStreamCaptor.capture()); when(snap.getStream(any(), any())).thenReturn(new ByteArrayInputStream(new byte[0])); when(jfr.takeSnapshot()).thenReturn(snap); JfrRecorder jfrRecorder = buildJfrRecorder(jfr); jfrRecorder.flushSnapshot(); - Path outputPath = pathCaptor.getValue(); - assertTrue(outputPath.startsWith(outDir)); + assertNotNull(inputStreamCaptor.getValue()); } @Test @@ -118,8 +116,7 @@ private JfrRecorder buildJfrRecorder(JFR jfr) { JfrRecorder.builder() .maxAgeDuration(maxAge) .settings(settings) - .namingConvention(new RecordingFileNamingConvention(outDir)) - .onNewRecordingFile(onNewRecordingFile) + .onNewRecording(onNewRecording) .jfr(jfr); return new JfrRecorder(builder) { diff --git a/profiler/src/test/java/com/splunk/opentelemetry/profiler/RecordingEscapeHatchTest.java b/profiler/src/test/java/com/splunk/opentelemetry/profiler/RecordingEscapeHatchTest.java deleted file mode 100644 index 1bb23188a..000000000 --- a/profiler/src/test/java/com/splunk/opentelemetry/profiler/RecordingEscapeHatchTest.java +++ /dev/null @@ -1,125 +0,0 @@ -/* - * Copyright Splunk Inc. - * - * 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 com.splunk.opentelemetry.profiler; - -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.ArgumentMatchers.isA; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -import java.io.IOException; -import java.nio.file.FileStore; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.time.Duration; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; -import java.util.stream.IntStream; -import java.util.stream.Stream; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; - -class RecordingEscapeHatchTest { - - public static final long PLENTY_OF_SPACE = 250 * 1024 * 1024L; - public static final long NOT_ENOUGH_SPACE = 29L; - Duration recordingDuration = Duration.ofSeconds(15); - Path outputPath = Paths.get("/some/output/path"); - RecordingEscapeHatch.FilesShim filesShim; - RecordingFileNamingConvention convention; - FileStore fileStore; - - @BeforeEach - void setup() throws Exception { - filesShim = mock(RecordingEscapeHatch.FilesShim.class); - convention = mock(RecordingFileNamingConvention.class); - fileStore = mock(FileStore.class); - when(convention.getOutputPath()).thenReturn(outputPath); - when(filesShim.getFileStore(outputPath)).thenReturn(fileStore); - when(filesShim.isRegularFile(isA(Path.class))).thenReturn(true); - doAnswer( - invocation -> { - Path path = invocation.getArgument(0); - return path.toString().matches("/path/to/file\\d+.jfr"); - }) - .when(convention) - .matches(isA(Path.class)); - } - - @Test - void testCanContinueHappyPath() throws Exception { - when(fileStore.getUsableSpace()).thenReturn(PLENTY_OF_SPACE); - Stream outputFiles = makeFiles(5); - CountDownLatch closeLatch = new CountDownLatch(1); - outputFiles = outputFiles.onClose(closeLatch::countDown); - when(filesShim.list(outputPath)).thenReturn(outputFiles); - RecordingEscapeHatch escapeHatch = buildEscapeHatch(false); - assertTrue(escapeHatch.jfrCanContinue()); - assertTrue(closeLatch.await(5, TimeUnit.SECONDS)); - } - - @Test - void testTooManyFilesOnDisk() throws Exception { - when(fileStore.getUsableSpace()).thenReturn(PLENTY_OF_SPACE); - Stream outputFiles = makeFiles(5000); - when(filesShim.list(outputPath)).thenReturn(outputFiles); - boolean keepFiles = false; - RecordingEscapeHatch escapeHatch = buildEscapeHatch(keepFiles); - assertFalse(escapeHatch.jfrCanContinue()); - } - - @Test - void testTooManyFilesOnDiskButKeepFilesSpecified() throws Exception { - when(fileStore.getUsableSpace()).thenReturn(PLENTY_OF_SPACE); - Stream outputFiles = makeFiles(5000); - when(filesShim.list(outputPath)).thenReturn(outputFiles); - RecordingEscapeHatch escapeHatch = buildEscapeHatch(true); - assertTrue(escapeHatch.jfrCanContinue()); - } - - @Test - void testNotEnoughFreeSpace() throws Exception { - when(fileStore.getUsableSpace()).thenReturn(NOT_ENOUGH_SPACE); - Stream outputFiles = makeFiles(5); - when(filesShim.list(outputPath)).thenReturn(outputFiles); - RecordingEscapeHatch escapeHatch = buildEscapeHatch(false); - assertFalse(escapeHatch.jfrCanContinue()); - } - - @Test - void testFileStoreException() throws Exception { - when(fileStore.getUsableSpace()).thenReturn(PLENTY_OF_SPACE); - when(filesShim.list(outputPath)).thenThrow(new IOException("KABOOM")); - RecordingEscapeHatch escapeHatch = buildEscapeHatch(false); - assertFalse(escapeHatch.jfrCanContinue()); - } - - private RecordingEscapeHatch buildEscapeHatch(boolean keepFiles) { - return RecordingEscapeHatch.builder() - .recordingDuration(recordingDuration) - .configKeepsFilesOnDisk(keepFiles) - .namingConvention(convention) - .filesShim(filesShim) - .build(); - } - - private Stream makeFiles(int num) { - return IntStream.range(0, num).mapToObj(i -> Paths.get("/path/to/file" + i + ".jfr")); - } -} diff --git a/profiler/src/test/java/com/splunk/opentelemetry/profiler/RecordingSequencerTest.java b/profiler/src/test/java/com/splunk/opentelemetry/profiler/RecordingSequencerTest.java index 2cdae61ca..16a866e05 100644 --- a/profiler/src/test/java/com/splunk/opentelemetry/profiler/RecordingSequencerTest.java +++ b/profiler/src/test/java/com/splunk/opentelemetry/profiler/RecordingSequencerTest.java @@ -20,7 +20,6 @@ import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.*; -import java.nio.file.Paths; import java.time.Duration; import java.util.Collections; import java.util.concurrent.CountDownLatch; @@ -36,11 +35,9 @@ class RecordingSequencerTest { Duration duration = Duration.ofMillis(10); @Mock JfrRecorder recorder; - @Mock RecordingEscapeHatch escapeHatch; @Test void canContinueNotStarted() { - when(escapeHatch.jfrCanContinue()).thenReturn(true); when(recorder.isStarted()).thenReturn(false); RecordingSequencer sequencer = buildSequencer(); sequencer.handleInterval(); @@ -50,7 +47,6 @@ void canContinueNotStarted() { @Test void canContinueAlreadyStarted() { - when(escapeHatch.jfrCanContinue()).thenReturn(true); when(recorder.isStarted()).thenReturn(true); RecordingSequencer sequencer = buildSequencer(); sequencer.handleInterval(); @@ -61,86 +57,22 @@ void canContinueAlreadyStarted() { @Test void startThroughFlushSequence() throws Exception { CountDownLatch latch = new CountDownLatch(3); - when(escapeHatch.jfrCanContinue()).thenReturn(true); recorder = new MockRecorder(latch); RecordingSequencer sequencer = buildSequencer(); sequencer.start(); assertTrue(latch.await(5, SECONDS)); } - @Test - void testUnhealthyNotStarted() { - when(escapeHatch.jfrCanContinue()).thenReturn(false); - when(recorder.isStarted()).thenReturn(false); - RecordingSequencer sequencer = buildSequencer(); - sequencer.handleInterval(); - verify(recorder, never()).start(); - verify(recorder, never()).flushSnapshot(); - } - - @Test - void testUnhealthyStartedCallsStop() { - when(escapeHatch.jfrCanContinue()).thenReturn(true); - when(recorder.isStarted()).thenReturn(false); - RecordingSequencer sequencer = buildSequencer(); - - // First time through we start - sequencer.handleInterval(); - verify(recorder).start(); - verify(recorder, never()).flushSnapshot(); - when(recorder.isStarted()).thenReturn(true); - - // Second time through we flush - sequencer.handleInterval(); - verify(recorder).flushSnapshot(); - verify(recorder, never()).stop(); - - // Now we are broken, so we call stop() - when(escapeHatch.jfrCanContinue()).thenReturn(false); - sequencer.handleInterval(); - verify(recorder).stop(); - verify(recorder, times(1)).flushSnapshot(); - verify(recorder, times(1)).start(); - } - - @Test - void testRecoversAfterSkipping() throws Exception { - CountDownLatch flushLatch = new CountDownLatch(3); - when(escapeHatch.jfrCanContinue()).thenReturn(true); - MockRecorder recorder = new MockRecorder(flushLatch); - RecordingSequencer sequencer = buildSequencer(recorder); - sequencer.start(); - assertTrue(flushLatch.await(5, SECONDS)); - assertEquals(1, recorder.stopLatch.getCount()); - assertTrue(recorder.isStarted()); - - // After 3 flushes, we are now broken! - when(escapeHatch.jfrCanContinue()).thenReturn(false); - assertTrue(recorder.stopLatch.await(5, SECONDS)); - assertFalse(recorder.isStarted()); - - flushLatch = new CountDownLatch(3); - recorder.setFlushLatch(flushLatch); - // And now we got happy again - when(escapeHatch.jfrCanContinue()).thenReturn(true); - assertTrue(flushLatch.await(5, SECONDS)); - assertTrue(recorder.isStarted()); - } - private RecordingSequencer buildSequencer() { return buildSequencer(recorder); } private RecordingSequencer buildSequencer(JfrRecorder recorder) { - return RecordingSequencer.builder() - .recordingEscapeHatch(escapeHatch) - .recordingDuration(duration) - .recorder(recorder) - .build(); + return RecordingSequencer.builder().recordingDuration(duration).recorder(recorder).build(); } private static class MockRecorder extends JfrRecorder { - private volatile CountDownLatch flushLatch; + private final CountDownLatch flushLatch; final CountDownLatch stopLatch = new CountDownLatch(1); boolean started; @@ -149,8 +81,7 @@ public MockRecorder(CountDownLatch flushLatch) { new Builder() .settings(Collections.emptyMap()) .maxAgeDuration(Duration.ofSeconds(10)) - .onNewRecordingFile(mock(Consumer.class)) - .namingConvention(new RecordingFileNamingConvention(Paths.get(".")))); + .onNewRecording(mock(Consumer.class))); this.flushLatch = flushLatch; started = false; } @@ -182,9 +113,5 @@ public void stop() { started = false; stopLatch.countDown(); } - - public void setFlushLatch(CountDownLatch flushLatch) { - this.flushLatch = flushLatch; - } } } From 6804c218bbcea174b8e2c62d93bc0cafb4278c78 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Tue, 2 May 2023 13:00:23 +0300 Subject: [PATCH 2/2] fix tests --- .../com/splunk/opentelemetry/profiler/JfrRecorderTest.java | 2 ++ .../splunk/opentelemetry/profiler/RecordingSequencerTest.java | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrRecorderTest.java b/profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrRecorderTest.java index 70d67d688..b47059c9d 100644 --- a/profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrRecorderTest.java +++ b/profiler/src/test/java/com/splunk/opentelemetry/profiler/JfrRecorderTest.java @@ -45,6 +45,7 @@ class JfrRecorderTest { Map settings; @Mock Recording recording; @Mock Consumer onNewRecording; + @Mock RecordingFileNamingConvention namingConvention; @BeforeEach void setup() { @@ -117,6 +118,7 @@ private JfrRecorder buildJfrRecorder(JFR jfr) { .maxAgeDuration(maxAge) .settings(settings) .onNewRecording(onNewRecording) + .namingConvention(namingConvention) .jfr(jfr); return new JfrRecorder(builder) { diff --git a/profiler/src/test/java/com/splunk/opentelemetry/profiler/RecordingSequencerTest.java b/profiler/src/test/java/com/splunk/opentelemetry/profiler/RecordingSequencerTest.java index 16a866e05..756e179a2 100644 --- a/profiler/src/test/java/com/splunk/opentelemetry/profiler/RecordingSequencerTest.java +++ b/profiler/src/test/java/com/splunk/opentelemetry/profiler/RecordingSequencerTest.java @@ -35,6 +35,7 @@ class RecordingSequencerTest { Duration duration = Duration.ofMillis(10); @Mock JfrRecorder recorder; + @Mock RecordingFileNamingConvention namingConvention; @Test void canContinueNotStarted() { @@ -71,7 +72,7 @@ private RecordingSequencer buildSequencer(JfrRecorder recorder) { return RecordingSequencer.builder().recordingDuration(duration).recorder(recorder).build(); } - private static class MockRecorder extends JfrRecorder { + private class MockRecorder extends JfrRecorder { private final CountDownLatch flushLatch; final CountDownLatch stopLatch = new CountDownLatch(1); boolean started; @@ -81,6 +82,7 @@ public MockRecorder(CountDownLatch flushLatch) { new Builder() .settings(Collections.emptyMap()) .maxAgeDuration(Duration.ofSeconds(10)) + .namingConvention(namingConvention) .onNewRecording(mock(Consumer.class))); this.flushLatch = flushLatch; started = false;