Obtain a checkpoint AppMap #91

kgilpin · 2021-07-27T18:05:07Z

A checkpoint AppMap returns an appmap JSON, but also leaves the current recording in progress.

This can be used to get an AppMap as an intermediate result, then continue the program execution without erasing the events that led up to that point.

Also:

Stream all events to a temp file; removing the Memory recorder.
Recording is returned as an object, which can be deleted, or renamed,
or read into a Writer.
ActiveSessionException is sometimes meaningful, but usually it's just
like a RuntimeException. So, a lot of this behavior has been removed.
The main explicit check that remains is to see if a recording is or is
not in progress when certain actions are performed. Other types of
unexpected errors are not client-caused and therefore they are just
runtime exceptions and will probably result in a 500 error to the
client.
Because events are written into a temp file, the name of the AppMap file
(if any), is not needed until the recording is stopped.

Resolves #78
Resolves #50
Resolves #44

Stream all events to a temp file; removing the Memory recorder. Recording is returned as an object, which can be deleted, or renamed, or read into a Writer. ActiveSessionException is sometimes meaningful, but usually it's just like a RuntimeException. So, a lot of this behavior has been removed. The main explicit check that remains is to see if a recording is or is not in progress when certain actions are performed. Other types of unexpected errors are not client-caused and therefore they are just runtime exceptions and will probably result in a 500 error to the client. Because events are written into a temp file, the name of the AppMap file (if any), is not needed until the recording is stopped.

…s access

return events don't duplicate the function call fields. http_server_request and sql_query don't need function call fields either. Delivers #50 Delivers #44

Setting System property appmap.debug=true also enables the other debug settings (hooks, http).

Method wrappers invoke base method as 'invokeWrappedMethod' rather than 'invoke'

parent_id is now set on 'return' events.

kgilpin · 2021-07-30T11:46:48Z

src/main/java/com/appland/appmap/Agent.java

@@ -67,10 +62,17 @@ public static void premain(String agentArgs, Instrumentation inst) {
      metadata.recorderName = "remote_recording";
      metadata.scenarioName = appmapName;

-      recorder.start(fileName, metadata);


fileName is not needed until stop.

kgilpin · 2021-07-30T11:47:09Z

src/main/java/com/appland/appmap/config/Properties.java

  public static final Boolean DebugLocals = (System.getProperty("appmap.debug.locals") != null);
+  public static final Boolean DebugHttp = Debug || System.getProperty("appmap.debug.http") != null;


Seems more intuitive to me to have appmap.debug also enable these settings.

kgilpin · 2021-07-30T11:47:35Z

src/main/java/com/appland/appmap/output/v1/Event.java

   */
-  public Event() {
-    this.setId(issueId());
+  public static Event functionCallEvent(Event master) {


Function calls need all this info, but returns don't.

kgilpin · 2021-07-30T11:49:05Z

src/main/java/com/appland/appmap/process/hooks/ToggleRecord.java

@@ -177,7 +201,7 @@ public static void doFilterJakarta(Event event, Object[] args) throws ExitEarly
        junit = true;
      }
    }
-    IRecordingSession.Metadata metadata = new IRecordingSession.Metadata();
+    RecordingSession.Metadata metadata = new RecordingSession.Metadata();


There's only one type of RecordingSession now - it streams events to a temp file.

kgilpin · 2021-07-30T11:49:24Z

src/main/java/com/appland/appmap/process/hooks/ToggleRecord.java

-      final String fileName = String.join("_", event.definedClass, event.methodId)
-              .replaceAll("[^a-zA-Z0-9-_]", "_");
-      IRecordingSession.Metadata metadata = getMetadata(event);
-      metadata.feature = identifierToSentence(event.methodId);


feature and featureGroup are deprecated.

kgilpin · 2021-07-30T11:50:17Z

src/main/java/com/appland/appmap/record/AppMapSerializer.java

  }

  /**
   * Closes outstanding JSON objects and closes the writer.
   * @throws IOException If a writer error occurs
   */
-  public void finalize() throws IOException {


We don't want to override finalize - it's used in garbage collection.

kgilpin · 2021-07-30T11:51:13Z

src/main/java/com/appland/appmap/record/Recorder.java

+    Event lastEvent;
+    // Avoid accepting new events on a thread that's already processing an event.
+    boolean isProcessing;
+    Stack<Event> callStack = new Stack<>();


This is used to populate parent_id.

kgilpin · 2021-07-30T11:51:30Z

src/main/java/com/appland/appmap/record/Recorder.java

-      throws ActiveSessionException {
-    if (this.hasActiveSession()) {
-      throw new ActiveSessionException(ERROR_SESSION_PRESENT);
+  static class ActiveSession {


Wrap the active session in an interface for safety and consistency.

kgilpin · 2021-07-30T11:52:45Z

src/main/java/com/appland/appmap/record/Recorder.java

+
+    ThreadState ts = threadState();
+
+    // We don't want re-entrant events on the same thread.


But I don't see why we'd need to block other threads here.

kgilpin · 2021-07-30T11:53:14Z

src/main/java/com/appland/appmap/record/Recorder.java

+        ts.callStack.push(event);
+      } else if ( event.event.equals("return") ) {
+        if ( ts.callStack.isEmpty() ) {
+          Logger.println("Discarding 'return' event because the call stack is empty for this thread");


This could happen if a recording is started and the first event that's recorded is a return.

kgilpin · 2021-07-30T11:56:47Z

src/main/java/com/appland/appmap/transform/annotations/CtClassUtil.java

+    return false;
+  }
+
+  private static <V> V tryClass(CtClass cls, String member, ClassAccessor<V> accessor) {


By testing each access, we don't abort the isChildOf entirely if one access attempt raises an exception.

kgilpin · 2021-07-30T11:57:11Z

src/main/java/com/appland/appmap/transform/annotations/CtClassUtil.java

    } catch (NotFoundException e) {
      if (Properties.DebugHooks) {
-        Logger.println("could not resolve class hierarchy");
-        Logger.println(e);
+        Logger.printf("NotFoundException resolving %s of class %s: %s\n", member, cls.getName(), e.getMessage());


If these messages are benign, we may want to suppress them even more.

kgilpin · 2021-07-30T11:58:33Z

test/entrypoint

@@ -27,7 +27,7 @@ ${@:-run_bats}
 bats_ret=$?

 kill ${JVM_PID}
-wait -f ${JVM_PID}


-f was causing a failure in Travis - not sure why.

Looks like -f is a new feature in bash 5.

kgilpin · 2021-07-30T11:59:05Z

test/smoke.bats

@@ -121,9 +122,8 @@ load 'helper'
  # Sanity check the events and classmap
  assert_json_eq '.events | length' 6

-  assert_json_eq '.classMap | length' 2
-  assert_json_eq '[.classMap[0] | recurse | .name?] | join(".")' javax.servlet.http.HttpServlet.service


HTTP server request no longer includes defined_class or method_id.

apotterri · 2021-07-30T14:01:14Z

FWIW, doesn't look like GH understands "Delivers", but there are some alternatives to link a PR to an issue: https://docs.github.com/en/github/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue .

apotterri

Code looks good. There's a fair bit of overlap among the commits, you might want to do some history cleanup before you merge.

apotterri · 2021-07-30T14:14:29Z

test/entrypoint

@@ -27,7 +27,7 @@ ${@:-run_bats}
 bats_ret=$?

 kill ${JVM_PID}
-wait -f ${JVM_PID}


Looks like -f is a new feature in bash 5.

kgilpin · 2021-07-30T18:51:18Z

I’m not sure how to do history cleanup efficiently. Believe it or not I did try and organize it some and this is the result. Are there tools that make it easy? Git CLI doesn’t seem to, particularly.

…

On Fri, Jul 30, 2021 at 1:32 PM Alan Potter ***@***.***> wrote: ***@***.**** approved this pull request. Code looks good. There's a fair bit of overlap among the commits, you might want to do some history cleanup before you merge. ------------------------------ In test/entrypoint <#91 (comment)>: > @@ -27,7 +27,7 @@ ${@:-run_bats} bats_ret=$? kill ${JVM_PID} -wait -f ${JVM_PID} Looks like -f is a new feature in bash 5. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#91 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVC63INR7QG4Z6JW55VQTT2LO4XANCNFSM5BCX5QQQ> .

apotterri · 2021-08-01T12:34:36Z

Are there tools that make it easy?

Not really, no. There are a number of GUI tools that say they let you do an interactive rebase, but they're really not any better than the command line....

If it was me, I'd consider doing git reset origin/master. This will throw away the current history, but leave you with all your changes in your working copy. You can then construct a new history by adding and committing subsets of the files. (I said "throw away", but, if you need to, you can easily recover the current history using git reflog.)

Lately I've been using Sourcetree to stage subsets of files before committing. I find it makes it easy to see and stage changes in files, and also to revert hunks if I need to. I haven't used it as much, but GitLens also seems pretty good.

appland-release · 2021-08-02T14:24:13Z

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

v1.3.0
GitHub release

Your semantic-release bot 📦🚀

cleanup: Optimize all imports

ae144a3

kgilpin force-pushed the feat/checkpoint branch from 216ebe2 to ae144a3 Compare July 28, 2021 12:06

kgilpin added 9 commits July 28, 2021 10:44

doc: Add instructions for debugging the bats tests

17863a1

refactor: Refactor recorder and snapshot code

878f732

fix: Remove '-f' option to 'wait'

637f904

fix: Improve robustness and logging of parent interface and superclas…

a24f056

…s access

feat: Apply different fields to call, return, sql, etc

3d8db1f

return events don't duplicate the function call fields. http_server_request and sql_query don't need function call fields either. Delivers #50 Delivers #44

feat: appmap.debug enables additional debug info

67e935e

Setting System property appmap.debug=true also enables the other debug settings (hooks, http).

cleanup: Rename delegated method for clarity

50a8dd5

Method wrappers invoke base method as 'invokeWrappedMethod' rather than 'invoke'

feat: Record return event parent_id

40be795

parent_id is now set on 'return' events.

kgilpin force-pushed the feat/checkpoint branch from 3a43931 to 40be795 Compare July 29, 2021 20:51

fix: Keep events 'open' until the next event is received

9e12099

kgilpin commented Jul 30, 2021

View reviewed changes

kgilpin marked this pull request as ready for review July 30, 2021 11:59

kgilpin requested a review from apotterri July 30, 2021 14:03

kgilpin requested a review from dustinbyrne July 30, 2021 14:03

test: Add system test for getting a checkpoint recording

f338582

apotterri approved these changes Jul 30, 2021

View reviewed changes

kgilpin merged commit 41cd257 into master Aug 2, 2021

kgilpin deleted the feat/checkpoint branch August 2, 2021 12:20

appland-release added the released label Aug 2, 2021

kgilpin mentioned this pull request Aug 4, 2021

feat: Issue 50 and 44 remove unnecessary info add parent id to events #54

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Obtain a checkpoint AppMap #91

Obtain a checkpoint AppMap #91

kgilpin commented Jul 27, 2021 •

edited

Loading

kgilpin Jul 30, 2021

kgilpin Jul 30, 2021

kgilpin Jul 30, 2021

kgilpin Jul 30, 2021

kgilpin Jul 30, 2021

kgilpin Jul 30, 2021

kgilpin Jul 30, 2021

kgilpin Jul 30, 2021 •

edited

Loading

kgilpin Jul 30, 2021

kgilpin Jul 30, 2021

kgilpin Jul 30, 2021

kgilpin Jul 30, 2021

kgilpin Jul 30, 2021

apotterri Jul 30, 2021

kgilpin Jul 30, 2021

apotterri commented Jul 30, 2021

apotterri left a comment

apotterri Jul 30, 2021

kgilpin commented Jul 30, 2021 via email

apotterri commented Aug 1, 2021 •

edited

Loading

appland-release commented Aug 2, 2021

		public static final Boolean DebugLocals = (System.getProperty("appmap.debug.locals") != null);
		public static final Boolean DebugHttp = Debug \|\| System.getProperty("appmap.debug.http") != null;


		ThreadState ts = threadState();

		// We don't want re-entrant events on the same thread.

Obtain a checkpoint AppMap #91

Obtain a checkpoint AppMap #91

Conversation

kgilpin commented Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgilpin Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apotterri commented Jul 30, 2021

apotterri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgilpin commented Jul 30, 2021 via email

apotterri commented Aug 1, 2021 • edited Loading

appland-release commented Aug 2, 2021

kgilpin commented Jul 27, 2021 •

edited

Loading

kgilpin Jul 30, 2021 •

edited

Loading

apotterri commented Aug 1, 2021 •

edited

Loading