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: Add language to appmap.yml #152

Merged
merged 2 commits into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ You can create `appmap.yml` config file; if not found, a default one will be cre
```yaml
name: application-name # from package.json by default
appmap_dir: tmp/appmap
language: javascript
packages:
- path: . # paths to instrument, relative to appmap.yml location
exclude: # code to exclude from instrumentation
Expand Down
19 changes: 19 additions & 0 deletions src/__tests__/__snapshots__/config.test.ts.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Config migrate does not update an existing config if the language field is already set 1`] = `
"name: test-package
language: ruby
appmap_dir: appmap
packages:
- path: .
"
`;

exports[`Config migrate sets the language field in existing configs 1`] = `
"name: test-package
appmap_dir: appmap
packages:
- path: .
language: javascript
"
`;
39 changes: 39 additions & 0 deletions src/__tests__/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ describe(Config, () => {
packages: new PackageMatcher("/test/app", [
{ path: ".", exclude: ["node_modules", ".yarn"] },
]),
language: "javascript",
});
});

Expand All @@ -41,6 +42,7 @@ describe(Config, () => {
root: fixMacOsTmpPath(dir),
relativeAppmapDir: "tmp/appmap",
appName: basename(dir),
language: "javascript",
});
});

Expand All @@ -53,6 +55,7 @@ describe(Config, () => {
root: fixMacOsTmpPath(dir),
relativeAppmapDir: "tmp/appmap",
appName: "test-package",
language: "javascript",
});
});

Expand All @@ -61,6 +64,7 @@ describe(Config, () => {
"appmap.yml",
YAML.stringify({
name: "test-package",
language: "javascript",
appmap_dir: "appmap",
packages: [{ path: ".", exclude: ["excluded"] }, "../lib"],
}),
Expand All @@ -71,6 +75,7 @@ describe(Config, () => {
expect(new Config()).toMatchObject({
root: fixMacOsTmpPath(dir),
relativeAppmapDir: "appmap",
language: "javascript",
appName: "test-package",
packages: new PackageMatcher(fixMacOsTmpPath(dir), [
{ path: ".", exclude: ["excluded"] },
Expand All @@ -84,6 +89,7 @@ describe(Config, () => {
"appmap.yml",
YAML.stringify({
name: "test-package",
language: "javascript",
appmap_dir: "appmap",
packages: [{ regexp: "foo", enabled: false }],
}),
Expand All @@ -95,6 +101,7 @@ describe(Config, () => {
root: fixMacOsTmpPath(dir),
relativeAppmapDir: "appmap",
appName: "test-package",
language: "javascript",
packages: new PackageMatcher(fixMacOsTmpPath(dir), [
{ path: ".", exclude: ["node_modules", ".yarn"] },
]),
Expand All @@ -112,6 +119,38 @@ describe(Config, () => {
expect(readFileSync("appmap.yml").toString()).toEqual(malformedAppMapFileContent);
});

describe("migrate", () => {
it("sets the language field in existing configs", () => {
writeFileSync(
"appmap.yml",
YAML.stringify({
name: "test-package",
appmap_dir: "appmap",
packages: [{ path: "." }],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a test of migration with user comments, to check they're preserved?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say skip this. Let's get this out the door.

}),
);
const config = new Config();
config.migrate();

expect(readFileSync("appmap.yml").toString()).toMatchSnapshot();
});

it("does not update an existing config if the language field is already set", () => {
const appmapYml = YAML.stringify({
name: "test-package",
language: "ruby",
appmap_dir: "appmap",
packages: [{ path: "." }],
});

writeFileSync("appmap.yml", appmapYml);
const config = new Config();
config.migrate();

expect(readFileSync("appmap.yml").toString()).toMatchSnapshot();
});
});

let dir: string;
beforeEach(() => {
chdir((dir = tmp.dirSync().name));
Expand Down
1 change: 1 addition & 0 deletions src/bin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export function main() {
info("Writing default config to %s", config.configPath);
writeFileSync(config.configPath, YAML.stringify(config));
} else info("Using config file %s", config.configPath);
config.migrate();
config.export();

// FIXME: Probably there should be a way to remove this altogether
Expand Down
55 changes: 41 additions & 14 deletions src/config.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
import assert from "node:assert";
import { readFileSync } from "node:fs";
import { readFileSync, writeFileSync } from "node:fs";
import { basename, join, resolve } from "node:path";
import { cwd } from "node:process";

import { PackageJson } from "type-fest";
import YAML from "yaml";
import YAML, { Document } from "yaml";
Copy link
Collaborator

Choose a reason for hiding this comment

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

You probably only need type Document here.


import { warn } from "./message";
import { info, warn } from "./message";
import PackageMatcher, { Package, parsePackages } from "./PackageMatcher";
import locateFileUp from "./util/findFileUp";
import lazyOpt from "./util/lazyOpt";
import tryOr from "./util/tryOr";
import { isNativeError } from "node:util/types";

const responseBodyMaxLengthDefault = 10000;
const kResponseBodyMaxLengthEnvar = "APPMAP_RESPONSE_BODY_MAX_LENGTH";
Expand All @@ -24,6 +23,10 @@ export class Config {
public readonly default: boolean;
public readonly packages: PackageMatcher;
public readonly responseBodyMaxLength: number;
public readonly language: string;

private readonly document?: Document;
private migrationPending = false;

constructor(pwd = cwd()) {
const configDir = locateFileUp("appmap.yml", process.env.APPMAP_ROOT ?? pwd);
Expand All @@ -35,13 +38,17 @@ export class Config {
const root = (this.root = process.env.APPMAP_ROOT ?? configDir ?? packageDir() ?? pwd);

this.configPath = join(root, "appmap.yml");
const config = readConfigFile(this.configPath);
this.default = !config;
this.document = loadConfigDocument(this.configPath);
const config = this.document ? readConfigFile(this.document) : undefined;
this.default = !this.document;

this.relativeAppmapDir = config?.appmap_dir ?? "tmp/appmap";

this.appName = config?.name ?? targetPackage()?.name ?? basename(root);

this.language = config?.language ?? "javascript";
this.migrationPending ||= !!config && config.language === undefined;

this.packages = new PackageMatcher(
root,
config?.packages ?? [
Expand Down Expand Up @@ -115,20 +122,38 @@ export class Config {
toJSON(): ConfigFile {
return {
name: this.appName,
language: this.language,
appmap_dir: this.relativeAppmapDir,
packages: this.packages,
};
}

migrate() {
if (!this.migrationPending) return;

if (this.document) {
this.document.set("language", this.language);
}

info("appmap.yml requires migration, changes will be automatically applied.");
writeFileSync(
this.configPath,
YAML.stringify(this.document ?? this, { keepSourceTokens: true }),
);
}
}

interface ConfigFile {
appmap_dir?: string;
name?: string;
packages?: Package[];
response_body_max_length?: number;
language?: string;
}

function readConfigFile(path: string | undefined): ConfigFile | undefined {
// Maintaining the YAML document is important to preserve existing comments and formatting
// in the original file.
function loadConfigDocument(path: string | undefined): Document | undefined {
if (!path) return;

let fileContent: string;
Expand All @@ -139,22 +164,24 @@ function readConfigFile(path: string | undefined): ConfigFile | undefined {
throw exn;
}

let config;
try {
config = YAML.parse(fileContent) as unknown;
} catch (exn) {
assert(isNativeError(exn));
const document = YAML.parseDocument(fileContent, { keepSourceTokens: true });
if (document.errors.length > 0) {
const errorMessage = document.errors.map((e) => `${e.name}: ${e.message}`).join("\n");
throw new Error(
`Error parsing config file at ${path}: ${exn.message}\nYou can remove the file to use the default configuration.`,
`Failed to parse config file at ${path}\n${errorMessage}\nYou can remove the file to use the default configuration.`,
);
}
if (!config) return;
return document;
}

function readConfigFile(document: Document): ConfigFile {
const config = document.toJSON() as Record<string, unknown>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't do a forced coercion like that. Instead, coerce to unknown and let asserts below carve the actual type.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be fair, TypeScript doesn't provide great options for this situation. When loading data from a file, there is not much alternative to type coercion.

Copy link
Collaborator

@dividedmind dividedmind May 22, 2024

Choose a reason for hiding this comment

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

That's not true. What I suggested is literally how to do it and it works really well:

const input: unknown = JSON.parse(inputText);
assert(input && typeof input === "object");
// typescript knows input is object here
if ("key" in input && typeof input.key === "string")
  process(input.key /* TypeScript knows it's a string here! */);

Copy link
Contributor

Choose a reason for hiding this comment

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

OK but I mean, so what? If the file is somehow mangled or corrupted, it's still going to error.

const result: ConfigFile = {};
assert(typeof config === "object");
if ("name" in config) result.name = String(config.name);
if ("appmap_dir" in config) result.appmap_dir = String(config.appmap_dir);
if ("packages" in config) result.packages = parsePackages(config.packages);
if ("language" in config) result.language = String(config.language);
if ("response_body_max_length" in config) {
const value = parseInt(String(config.response_body_max_length));
result.response_body_max_length = value >= 0 ? value : undefined;
Expand Down
1 change: 1 addition & 0 deletions test/__snapshots__/simple.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

exports[`creating a default config file 1`] = `
"name: test
language: javascript
appmap_dir: tmp/appmap
packages:
- path: .
Expand Down
3 changes: 2 additions & 1 deletion test/codeBlock/appmap.yml
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
name: codeBlock
appmap_dir: tmp/appmap
appmap_dir: tmp/appmap
language: javascript
7 changes: 4 additions & 3 deletions test/functionLabels/appmap.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name: function-labels-appmap-node-test
appmap_dir: tmp/appmap
packages:
packages:
- path: .
- module: node:console
shallow: true
Expand All @@ -12,6 +12,7 @@ packages:
- name: log
label: logging
- name: debug
labels:
labels:
- logging
- debugging
- debugging
language: javascript
1 change: 1 addition & 0 deletions test/httpClient/appmap.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
name: http-client-appmap-node-test
appmap_dir: tmp/appmap
response_body_max_length: 50
language: javascript
1 change: 1 addition & 0 deletions test/httpServer/appmap.yml
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
name: http-server-appmap-node-test
appmap_dir: tmp/appmap
language: javascript
1 change: 1 addition & 0 deletions test/jest/appmap.yml
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
name: jest-appmap-node-test
appmap_dir: tmp/appmap
language: javascript
5 changes: 3 additions & 2 deletions test/libraryCalls/appmap.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ packages:
shallow: true
- module: node:console # works with or without node: qualifier
shallow: true
exclude:
exclude:
- warn
- assert # exclude because this is captured only in windows
- assert # exclude because this is captured only in windows
language: javascript
1 change: 1 addition & 0 deletions test/mocha/appmap.yml
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
name: mocha-appmap-node-test
appmap_dir: tmp/appmap
language: javascript
1 change: 1 addition & 0 deletions test/mysql/appmap.yml
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
name: mysql-appmap-node-test
appmap_dir: tmp/appmap
language: javascript
1 change: 1 addition & 0 deletions test/next/appmap.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ packages:
exclude:
- node_modules
- .yarn
language: javascript
3 changes: 2 additions & 1 deletion test/prisma/appmap.yml
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
name: prisma-appmap-node-test
appmap_dir: tmp/appmap
appmap_dir: tmp/appmap
language: javascript
9 changes: 5 additions & 4 deletions test/simple/appmap.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
name: simple
appmap_dir: tmp/appmap
packages:
- path: .
exclude:
- skipped
- A.skippedMethod
- path: .
exclude:
- skipped
- A.skippedMethod
language: javascript
1 change: 1 addition & 0 deletions test/sqlite/appmap.yml
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
name: sqlite-appmap-node-test
appmap_dir: tmp/appmap
language: javascript
1 change: 1 addition & 0 deletions test/typescript-esm/appmap.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ packages:
exclude:
- node_modules
- .yarn
language: javascript
1 change: 1 addition & 0 deletions test/typescript/appmap.yml
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
name: appmap-node
appmap_dir: tmp/appmap
language: javascript
1 change: 1 addition & 0 deletions test/vitest/appmap.yml
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
name: vitest-appmap-node-test
appmap_dir: tmp/appmap
language: javascript