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

marcuss · 2021-02-27T14:20:58Z

Contains changes from PR: #48

And changes to fix issue 50

…this prevent using custom serialization and it's the start of a bigger refactor to an Event hierarchy.

feat: AppMap Maven plugin (getappmap#46)

…this prevent using custom serialization and it's the start of a bigger refactor to an Event hierarchy.

…uss/appmap-java into issue-44-duplicate-event-properties

Added proper unit test for both. Minor code clean up.

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

dustinbyrne · 2021-03-05T17:13:24Z

@marcuss, the integrationTest task is failing. Any ideas?

marcuss · 2021-03-09T21:56:53Z

@dustinbyrne I will try to reproduce it and check!

removed unnecessary path = null line.

marcuss · 2021-03-09T22:17:43Z

@dustinbyrne integration test working now

…ck to select parent ids of events.

marcuss · 2021-03-14T00:20:09Z

@dustinbyrne I didn't check this PR after fixing the last comment, but I found another issue and corrected it, please take a look

…d-parent_id-to-events

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

dustinbyrne

Could you give this a try with the Spring Pet Clinic?

I'm running with the following command and getting errors:

mvn -DargLine="-javaagent:~/dev/appmap-java/build/libs/appmap-0.5.2.jar" test

I believe they're all related to HTTP server requests.

java.lang.IllegalArgumentException: Missing parent Event.

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

dustinbyrne · 2021-04-16T20:06:13Z

Hi @marcuss what happened with this PR?

marcuss · 2021-04-18T08:15:25Z

@dustinbyrne now that I look closely I think I close the wrong one, I remember you fixed an issue while I was working on it, and I though it was this one, let me take a look a reopened if this is still needed.

…ecessary-info-add-parent_id-to-events

marcuss · 2021-04-18T08:43:45Z

@dustinbyrne Please check out the updated PR.

dustinbyrne · 2021-04-20T18:44:11Z

There's still a few failure cases in Spring Pet Clinic. Unfortunately, I don't have the time to dig in to this deeper atm. The error is pretty vague, so it'll likely require a debugger:

org.springframework.web.util.NestedServletException: Request processing failed; nested exception is java.util.NoSuchElementException: No value present
	at org.springframework.samples.petclinic.web.PetControllerTests.testProcessUpdateFormSuccess(PetControllerTests.java:99)

marcuss · 2021-04-21T13:25:17Z

@dustinbyrne I haven't check the spring pet clinic error actually, I think this was the original mishap that made me overlook this PR, I will check this today to see if I can reproduce it

marcuss · 2021-04-22T04:22:19Z

@dustinbyrne I checked using the maven plugin, and there is no error, I even created locally a 1.0.5 version of the plugin and the agent just to be 100% sure the maven plugin was using the new version and verified too, reading the argument line generated, and yes it is using the version from this branch:


[INFO] --- appmap-maven-plugin:1.1.1:prepare-agent (default) @ spring-petclinic ---
[INFO] Initializing AppLand AppMap Java Recorder. with agent 1.0.5
[INFO] argLine set to -Dappmap.event.valueSize=1024 -Dappmap.config.file=appmap.yml -Dappmap.output.directory=target\appmap -Dappmap.debug.file=target\appmap\agent.log -Dappmap.debug -javaagent:C:\Users\msanchez3\.m2\repository\org\jacoco\org.jacoco.agent\0.8.5\org.jacoco.agent-0.8.5-runtime.jar=destfile=C:\code\marcuss\spring-petclinic\target\jacoco.exec -javaagent:C:\Users\msanchez3\.m2\repository\com\appland\appmap-agent\1.0.5\appmap-agent-1.0.5.jar=com.appland.appmap.LoadJavaAppMapAgentMojo@6e847a7

marcuss · 2021-04-22T04:30:59Z

@dustinbyrne I also checked using your way, and this command:

mvn -DargLine="-javaagent:C:\Users\marcuss\.m2\repository\com\appland\appmap-agent\1.0.5\appmap-agent-1.0.5.jar" test

And got again successful runs, check the generated files:
generated files

marcuss · 2021-04-22T04:32:08Z

@dustinbyrne are you sure you are using the correct java agent version? and by that, I meant the version generated with this branch.

marcuss · 2021-04-22T04:35:17Z

@dustinbyrne @ptrdvrk
btw the integration tests pass locally but the CI build says:

A problem occurred evaluating root project 'appmap-java'.
> Could not get unknown property 'ossrhPassword' for Credentials [username: appland.release] of type org.gradle.internal.credentials.DefaultPasswordCredentials_Decorated.

marcuss · 2021-04-22T04:36:10Z

@dustinbyrne @ptrdvrk
btw the integration tests pass locally but the CI build says:

A problem occurred evaluating root project 'appmap-java'.
> Could not get unknown property 'ossrhPassword' for Credentials [username: appland.release] of type org.gradle.internal.credentials.DefaultPasswordCredentials_Decorated.

dustinbyrne

Looking good! I've added a couple of small items to check out.

Additionally, would you mind running Checkstyle over these changes? We use the default Google style. I'll have to find out how we can better integrate Checkstyle into the project moving forward.

src/main/java/com/appland/appmap/util/EventUtil.java

dustinbyrne · 2021-04-28T16:24:59Z

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

+  public boolean isParentEventOf(Event other) {
+    return this.definedClass.equals(other.definedClass)
+            && this.methodId.equals(other.methodId)
+            && this.lineNumber.equals(other.lineNumber);
+  }


isSameMethod may be a more apt name. isParentEventOf can be misleading because it doesn't actually check the parent id.

by the moment this is used, the parent id property is null in the child, so we can not check for it, but if the defined class, the method id and the line number are the same Bingo!
could it be named: "isSameCodeAs" ???

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

dustinbyrne · 2021-04-28T17:09:00Z

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

+        if (event.parentId == null) {
+            this.classReferences.add(
+                    event.definedClass +
+                            ":" + event.methodId +
+                            ":" + event.isStatic +
+                            ":" + event.lineNumber);


Since these properties may now be null, maybe the condition should instead check for presence of methodId/isStatic/lineNumber for better future-proofing. This is also fine as it is currently.

If it is fine at it is I'll rather keep it since right now the intent is clearer, we are checking if the event does not have a parent, the other way could be misleading.

dustinbyrne · 2021-04-28T17:16:18Z

@dustinbyrne @ptrdvrk
btw the integration tests pass locally but the CI build says:

A problem occurred evaluating root project 'appmap-java'.
> Could not get unknown property 'ossrhPassword' for Credentials [username: appland.release] of type org.gradle.internal.credentials.DefaultPasswordCredentials_Decorated.

I've added an issue to make sure we're not attempting to publish in a non-master branch.

kgilpin · 2021-08-04T21:16:34Z

Closed via #91

marcuss added 13 commits February 18, 2021 22:08

Removed unwanted fields by changing the setters of the event method, …

67330fc

…this prevent using custom serialization and it's the start of a bigger refactor to an Event hierarchy.

fixed return event duplicate fields as well.

3d2c84b

fixed error of test in windows

8b76bec

Organize imports

70bfebd

Fixed the failing windows tests correctly.

12c3eb8

Merge pull request #1 from applandinc/master

3409ec9

feat: AppMap Maven plugin (getappmap#46)

Removed unwanted fields by changing the setters of the event method, …

3787ca4

…this prevent using custom serialization and it's the start of a bigger refactor to an Event hierarchy.

fixed return event duplicate fields as well.

de24b5b

fixed error of test in windows

037b850

Organize imports

2e08f72

Fixed the failing windows tests correctly.

cfa2f84

Merge branch 'issue-44-duplicate-event-properties' of github.com:marc…

8e2230b

…uss/appmap-java into issue-44-duplicate-event-properties

Mergerd issues 44 and 50 in the same PR.

0b8dd3d

Added proper unit test for both. Minor code clean up.

marcuss mentioned this pull request Feb 27, 2021

[Blocked by #50] Fixed Issue 44 Duplicate Event Properties #48

Closed

Using parent event to create key for recording session.

4c59f9d

marcuss commented Mar 2, 2021

View reviewed changes

src/main/java/com/appland/appmap/record/RecordingSessionGeneric.java Outdated Show resolved Hide resolved

marcuss marked this pull request as ready for review March 5, 2021 03:22

Fixed failing integration tests.

85ee38c

removed unnecessary path = null line.

marcuss changed the title ~~Issue 50 and 44 remove unnecessary info add parent id to events~~ [READY TO MERGE] Issue 50 and 44 remove unnecessary info add parent id to events Mar 9, 2021

marcuss added 2 commits March 13, 2021 18:54

Fixed failing integration tests, now the recorder preserve a call sta…

4dc966e

…ck to select parent ids of events.

Fixed failing integration tests, now the recorder preserve a call sta…

bb3d877

…ck to select parent ids of events.

marcuss added 2 commits March 13, 2021 19:23

Bump version to 0.5.2

57ef6b0

Merge branch 'master' into issue-50-and-44-remove-unnecessary-info-ad…

ab4e788

…d-parent_id-to-events

marcuss commented Mar 15, 2021

View reviewed changes

src/main/java/com/appland/appmap/record/Recorder.java Show resolved Hide resolved

dustinbyrne force-pushed the master branch 2 times, most recently from 4cffa84 to 991680a Compare March 16, 2021 21:31

dustinbyrne requested changes Mar 16, 2021

View reviewed changes

src/main/java/com/appland/appmap/record/RecordingSessionGeneric.java Outdated Show resolved Hide resolved

dustinbyrne force-pushed the master branch 3 times, most recently from f8202dc to ea9c595 Compare March 17, 2021 04:52

marcuss closed this Apr 9, 2021

marcuss reopened this Apr 18, 2021

marcuss changed the title ~~[READY TO MERGE] Issue 50 and 44 remove unnecessary info add parent id to events~~ feat: Issue 50 and 44 remove unnecessary info add parent id to events Apr 18, 2021

marcuss added 2 commits April 18, 2021 03:32

Merge remote-tracking branch 'origin' into issue-50-and-44-remove-unn…

b881a69

…ecessary-info-add-parent_id-to-events

Remove potentially dangerous exception throwing.

19e376d

Issue#75 Fix Appmap generated file name too long

12daea2

dustinbyrne requested changes Apr 28, 2021

View reviewed changes

dustinbyrne mentioned this pull request Apr 28, 2021

non-master branch attempts publish task #77

Closed

Addressed PR Comments from code review.

ff65976

kgilpin closed this Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

marcuss commented Feb 27, 2021 •

edited

Loading

dustinbyrne commented Mar 5, 2021

marcuss commented Mar 9, 2021

marcuss commented Mar 9, 2021

marcuss commented Mar 14, 2021 •

edited

Loading

dustinbyrne left a comment

dustinbyrne commented Apr 16, 2021

marcuss commented Apr 18, 2021

marcuss commented Apr 18, 2021

dustinbyrne commented Apr 20, 2021

marcuss commented Apr 21, 2021

marcuss commented Apr 22, 2021

marcuss commented Apr 22, 2021

marcuss commented Apr 22, 2021

marcuss commented Apr 22, 2021

marcuss commented Apr 22, 2021

dustinbyrne left a comment

dustinbyrne Apr 28, 2021

marcuss May 1, 2021

dustinbyrne Apr 28, 2021

marcuss May 1, 2021

dustinbyrne commented Apr 28, 2021

kgilpin commented Aug 4, 2021

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

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

Conversation

marcuss commented Feb 27, 2021 • edited Loading

dustinbyrne commented Mar 5, 2021

marcuss commented Mar 9, 2021

marcuss commented Mar 9, 2021

marcuss commented Mar 14, 2021 • edited Loading

dustinbyrne left a comment

Choose a reason for hiding this comment

dustinbyrne commented Apr 16, 2021

marcuss commented Apr 18, 2021

marcuss commented Apr 18, 2021

dustinbyrne commented Apr 20, 2021

marcuss commented Apr 21, 2021

marcuss commented Apr 22, 2021

marcuss commented Apr 22, 2021

marcuss commented Apr 22, 2021

marcuss commented Apr 22, 2021

marcuss commented Apr 22, 2021

dustinbyrne left a comment

Choose a reason for hiding this comment

dustinbyrne Apr 28, 2021

Choose a reason for hiding this comment

marcuss May 1, 2021

Choose a reason for hiding this comment

dustinbyrne Apr 28, 2021

Choose a reason for hiding this comment

marcuss May 1, 2021

Choose a reason for hiding this comment

dustinbyrne commented Apr 28, 2021

kgilpin commented Aug 4, 2021

marcuss commented Feb 27, 2021 •

edited

Loading

marcuss commented Mar 14, 2021 •

edited

Loading