Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Updated Documentation, Added validate-config goal to check on config files, and clean output directory action to delete previously generated files. #5

Merged
merged 19 commits into from
May 10, 2021

Conversation

marcuss
Copy link
Contributor

@marcuss marcuss commented Apr 11, 2021

Added validate-config goal to check on config files, and clean output directory action to delete previously generated files.

@dustinbyrne @ptrdvrk

@marcuss
Copy link
Contributor Author

marcuss commented Apr 11, 2021

to review the changes first commit the pending PR #2 to master.

@marcuss
Copy link
Contributor Author

marcuss commented Apr 23, 2021

@dustinbyrne @ptrdvrk could you check this one? I believe is ready to merge!

@marcuss marcuss changed the title feat: Added validate-config goal to check on config files, and clean output directory action to delete previously generated files. feat: Updated Documentation, Added validate-config goal to check on config files, and clean output directory action to delete previously generated files. Apr 27, 2021
Copy link
Contributor

@ptrdvrk ptrdvrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review and address the inline comments. Thank you!

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
use $buildDir whenever possible.
Copy link
Contributor

@dustinbyrne dustinbyrne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @marcuss! I've added some minor items regarding debug flags.

this.configFile = project.getObjects().fileProperty().fileValue(new File("appmap.yml"));
this.outputDirectory = project.getObjects().directoryProperty().fileValue(new File(DEFAULT_OUTPUT_DIRECTORY));
try {
LOGGER.setLevel(Level.parse(debug.toUpperCase()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appmap.debug won't match up with a logger level (other than INFO). It's expected to be a comma separated list of subsystems to enable verbose logging (hooks,http,locals).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, but then I should just put logger at INFO level, and omit this until an issue identifying what to log at hooks, HTTP and locals should log, right?

Comment on lines 48 to 58
String builder = "-javaagent:" +
RelativePathUtil.relativePath(
appmap.project.getProjectDir(), appmap.getAgentConf().getSingleFile()
);
List<String> argumentLn = ImmutableList.of(
builder,
"-Dappmap.config.file=" + appmap.getConfigFile().get().toString(),
"-Dappmap.output.directory=" + appmap.getOutputDirectory().get().toString(),
"-Dappmap.event.valueSize=" + appmap.getEventValueSize(),
"-Dappmap.debug=" + appmap.getDebug()
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the Maven logic for handling the debug flags:
https://github.com/applandinc/appmap-maven-plugin/blob/master/src/main/java/com/appland/appmap/AppMapAgentMojo.java#L101-L111

In summary: presence of any non-null, non-empty value will introduce -Dappmap.debug, -Dappmap.debug.file=${debugFile}. The value of debug will then be split by comma (the Maven plugin also allows for whitespace but doesn't document it), and appends -Dappmap.debug.hooks, -Dappmap.debug.locals, -Dappmap.debug.http as appropriate if the last token is included in the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the changes requested, but let me do some testing first, first time Monday I will address this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dustinbyrne @ptrdvrk finally got to test this properly, there is an issue with the gradle plugin that should be solved once is properly mad public, for now if you want to play with it to you can only do it with groovy gradle scripts (build.gradle) but not with kotlin DSL bild scripts (build.gradle.kts)

For testing I used:
https://github.com/spring-guides/tut-spring-boot-kotlin

but had to change the build.gradle.kts to build.gradle because of the aforementioned, you can use it if you like:

buildscript {
	repositories {
		mavenLocal()
	}
	dependencies {
		classpath "com.appland:appmap-gradle-plugin:1.0.3"
	}
}

plugins {
	id "org.springframework.boot" version "2.4.4"
	id "io.spring.dependency-management" version "1.0.11.RELEASE"
	id "org.jetbrains.kotlin.jvm" version "1.4.32"
	id "org.jetbrains.kotlin.plugin.spring" version "1.4.32"
	id "org.jetbrains.kotlin.plugin.allopen" version "1.4.32"
	id "org.jetbrains.kotlin.plugin.jpa" version "1.4.32"
	id "org.jetbrains.kotlin.kapt" version "1.4.32"
}

group = "com.example"
version = "0.0.1-SNAPSHOT"
java.sourceCompatibility = JavaVersion.VERSION_1_8

repositories {
	mavenCentral()
}

dependencies {
	implementation "org.springframework.boot:spring-boot-starter-data-jpa"
	implementation "org.springframework.boot:spring-boot-starter-mustache"
	implementation "org.springframework.boot:spring-boot-starter-web"
	implementation "com.fasterxml.jackson.module:jackson-module-kotlin"
	implementation "org.jetbrains.kotlin:kotlin-reflect"
	implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8"
	runtimeOnly "com.h2database:h2"
	runtimeOnly "org.springframework.boot:spring-boot-devtools"
	kapt "org.springframework.boot:spring-boot-configuration-processor"
	testImplementation('org.springframework.boot:spring-boot-starter-test') {
		exclude group: 'org.junit.vintage', module: 'junit-vintage-engine'
		exclude module: 'mockito-core'
	}
	testImplementation "org.junit.jupiter:junit-jupiter-api"
	testRuntimeOnly "org.junit.jupiter:junit-jupiter-engine"
	testImplementation "com.ninja-squad:springmockk:3.0.1"
	test.useJUnitPlatform()
}

compileKotlin {
	kotlinOptions.freeCompilerArgs =  ['-Xjsr305=strict']
	kotlinOptions.jvmTarget = "1.8"
}

allOpen {
	annotation("javax.persistence.Entity")
	annotation("javax.persistence.Embeddable")
	annotation("javax.persistence.MappedSuperclass")
}

apply plugin: 'com.appland.appmap'

appmap {
	configFile = file("$projectDir/appmap.yml")
	outputDirectory = file("$buildDir/appmap3")
	skip = false
	debug = "info"
	eventValueSize = 1024
}

Once you validate this, please merge

Thanks!

@dustinbyrne dustinbyrne merged commit 8f44c51 into getappmap:master May 10, 2021
@marcuss marcuss deleted the issue-3 branch May 11, 2021 02:17
appland-release pushed a commit that referenced this pull request May 11, 2021
# 1.0.0 (2021-05-11)

### Features

* Updated Documentation, Added validate-config goal to check on config files, and clean output directory action to delete previously generated files. ([#5](#5)) ([8f44c51](8f44c51))
@appland-release
Copy link

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

appland-release pushed a commit that referenced this pull request May 11, 2021
# 1.0.0 (2021-05-11)

### Features

* Updated Documentation, Added validate-config goal to check on config files, and clean output directory action to delete previously generated files. ([#5](#5)) ([8f44c51](8f44c51))
@appland-release
Copy link

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

appland-release pushed a commit that referenced this pull request May 12, 2021
# 1.0.0 (2021-05-12)

### Features

* Updated Documentation, Added validate-config goal to check on config files, and clean output directory action to delete previously generated files. ([#5](#5)) ([8f44c51](8f44c51))
@appland-release
Copy link

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants