Print diagnose report from CLI #401

jeffkreeftmeijer · 2021-06-24T07:23:55Z

In order to get the CLI diagnose command up to speed with the output from the Ruby and Elixir integrations, we created https://github.com/appsignal/diagnose_tests, which is an specification that describes the desired output to use as a guide and check for the diagnose output. It's modeled after the diagnose command in Ruby, and will eventually be used to test and develop diagnose commands for all integrations.

This patch introduces diagnose_tests to the Node.js library, and implements a new diagnose command that matches the specification. Because of differences in the generated JSON reports, the diagnose output in the Node.js library is not an exact match with the Ruby integration. For those cases, the diagnose_tests specification has specific assertions for the Node.js. The goal is to eventually get rid of those, but we’re cutting this project here to be able to ship the new diagnose now, and have it checked on CI going forward.

Note: this patch depends on #402

thijsc

Nice!

backlog-helper · 2021-06-30T07:01:12Z

While performing the daily checks some issues were found with this Pull Request.

This Pull Request needs more reviews. @tombruijn - (More info)

New issue guide | Backlog management | Rules | Feedback

tombruijn

Great on the human friendly output! Could you add a separate changeset for the updated output?

I notice some duplicate commits from #402 merged into main in this PR as well. Does it need a rebase?

Will the diagnose_tests repo also test the JSON payload in the future?

tombruijn · 2021-06-30T14:00:38Z

packages/nodejs/src/config/configmap.ts

@@ -61,6 +61,7 @@ export const JS_TO_RUBY_MAPPING: { [key: string]: string } = {
  debug: "debug",
  log: "log",
  logPath: "log_path",
+  logFilePath: "log_file_path",


Is this line what adds it to the diagnose report as a config option? Because we shouldn't include it as a config option in the sent report, it's a config option users can directly set (only through the logPath config option). The path itself is already part of the paths section which that contains more details.

What I see in a sent report:

This line was added because the diagnose command uses the JS_TO_RUBY_MAPPING object to fetch the names for each field. Because there was no name for the logFilePath field, it printed undefined: '/tmp/appsignal.log' before this change.

In Ruby, the log_path and log_file_path aren't printed in the configuration at all.I'll update the test in diagnose_tests to make sure this isn't the case in Node.js either, so we can remove this line again.

tombruijn · 2021-06-30T14:05:53Z

.semaphore/semaphore.yml

+            - cache restore packages-$SEMAPHORE_GIT_SHA-$SEMAPHORE_GIT_BRANCH-v$NODE_VERSION
+            - git submodule init
+            - git submodule update
+            - LANGUAGE=nodejs test/integration/diagnose/bin/test


Can we disable the debug mode in the diagnose tests now? I see a lot of debug output there.

tombruijn · 2021-07-01T15:28:00Z

@jeffkreeftmeijer we'll need to rebase this on the build matrix. Let me know if you need help with that.

jeffkreeftmeijer · 2021-07-02T06:48:34Z

@tombruijn I’m working on some support issues today, so it would be nice if you could pick this up. If you don’t have time, I’ll do it early next week.

tombruijn · 2021-07-02T08:54:02Z

build_matrix.yml

+        diagnose:
+          - git submodule init
+          - git submodule update
+          - LANGUAGE=nodejs test/integration/diagnose/bin/test


@jeffkreeftmeijer I've rebased this branch on the build matrix. I've added the diagnose tests as an "extra test" for the node.js package so it gets run for every node.js version as well.

jeffkreeftmeijer · 2021-07-06T08:15:28Z

I’ve updated this pull request to remove the logFilePath field and remove the debug output on CI.

tombruijn · 2021-07-06T09:15:58Z

packages/nodejs/src/cli/diagnose.ts

+    console.log(`Validation`)
+    console.log(
+      `  Validating Push API key: ${this.colorize(
+        data["validation"]["push_api_key"]


Not sure if it's in scope but I noticed that this output isn't accurate.

Given this config output (with an undefined push_api_key, which I also don't know if we should print it as undefined):

Configuration ... push_api_key: undefined

I get this validation:

Validation Validating Push API key: valid

See also the admin view of this report: https://appsignal.com/admin/diagnose_reports/60e41eb35ac13f2d186f711f

That should be caused by #408, which causes the result to return “valid” for any input. I’ve created a separate issue to fix that to make sure this moves along.

packages/nodejs/src/cli/diagnose.ts

tombruijn

See previous comments, GitHub still shows me I have approved it 🤷‍♂️

Co-authored-by: Tom de Bruijn <[email protected]>

tombruijn · 2021-07-07T12:14:54Z

Can you add a changeset for this PR? Something about diagnose report improvements.

jeffkreeftmeijer self-assigned this Jun 24, 2021

jeffkreeftmeijer added the enhancement label Jun 24, 2021

jeffkreeftmeijer force-pushed the diagnose branch 4 times, most recently from a3bd76b to 4fc72f5 Compare June 25, 2021 08:53

jeffkreeftmeijer requested review from tombruijn and thijsc June 28, 2021 06:09

jeffkreeftmeijer marked this pull request as ready for review June 28, 2021 06:09

thijsc approved these changes Jun 28, 2021

View reviewed changes

jeffkreeftmeijer mentioned this pull request Jun 29, 2021

Run diagnose command specification on CI appsignal/appsignal-ruby#735

Merged

tombruijn approved these changes Jun 30, 2021

View reviewed changes

tombruijn reviewed Jun 30, 2021

View reviewed changes

tombruijn force-pushed the diagnose branch from 4fc72f5 to 7fd6c8f Compare July 2, 2021 08:48

tombruijn reviewed Jul 2, 2021

View reviewed changes

jeffkreeftmeijer force-pushed the diagnose branch 2 times, most recently from 6797c84 to 8d9b7bf Compare July 5, 2021 11:26

jeffkreeftmeijer added 10 commits July 5, 2021 13:41

Add diagnose integration tests

f89ced3

Run diagnose integration tests on CI

91fe9c6

Add cli/diagnose with header and library section

0f1c1c3

Add extension installation report

004d3c8

Add configuration section to diagnose

4e0ff6a

Add config diagnose link

6360869

Add API key validation section

cc26d7b

Add the paths section

1775ff6

Add diagnose reports section

adb5c79

Add diagnostics report section

64e4126

jeffkreeftmeijer added 7 commits July 5, 2021 13:41

Restore diagnose report sending

f1f0a08

Move report sending to separate function

9a47aab

Restore --(no-)send-report options

90aa7f7

Update diagnose_tests to remove log paths from Node.js diagnose

2798db4

Print specific list of configuration values in diagnose

dacae90

Remove logFilePath from JS_TO_RUBY_MAPPING

bde04e3

Add linux arm override to diagnose

ce945db

jeffkreeftmeijer force-pushed the diagnose branch from 8d9b7bf to ce945db Compare July 5, 2021 11:42

Update diagnose_tests to silence debug output on CI

645290b

jeffkreeftmeijer requested a review from tombruijn July 6, 2021 08:15

tombruijn reviewed Jul 6, 2021

View reviewed changes

packages/nodejs/src/cli/diagnose.ts Outdated Show resolved Hide resolved

tombruijn requested changes Jul 6, 2021

View reviewed changes

Update packages/nodejs/src/cli/diagnose.ts

1fea809

Co-authored-by: Tom de Bruijn <[email protected]>

jeffkreeftmeijer requested a review from tombruijn July 7, 2021 08:27

tombruijn approved these changes Jul 7, 2021

View reviewed changes

Add changeset for diagnose

f4501b0

jeffkreeftmeijer merged commit 87155be into main Jul 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print diagnose report from CLI #401

Print diagnose report from CLI #401

jeffkreeftmeijer commented Jun 24, 2021 •

edited

Loading

thijsc left a comment

backlog-helper bot commented Jun 30, 2021

tombruijn left a comment

tombruijn Jun 30, 2021 •

edited

Loading

jeffkreeftmeijer Jul 5, 2021 •

edited

Loading

tombruijn Jun 30, 2021

tombruijn commented Jul 1, 2021

jeffkreeftmeijer commented Jul 2, 2021

tombruijn Jul 2, 2021

jeffkreeftmeijer commented Jul 6, 2021

tombruijn Jul 6, 2021

jeffkreeftmeijer Jul 7, 2021

tombruijn left a comment •

edited

Loading

tombruijn commented Jul 7, 2021

Print diagnose report from CLI #401

Print diagnose report from CLI #401

Conversation

jeffkreeftmeijer commented Jun 24, 2021 • edited Loading

thijsc left a comment

Choose a reason for hiding this comment

backlog-helper bot commented Jun 30, 2021

tombruijn left a comment

Choose a reason for hiding this comment

tombruijn Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

jeffkreeftmeijer Jul 5, 2021 • edited Loading

Choose a reason for hiding this comment

tombruijn Jun 30, 2021

Choose a reason for hiding this comment

tombruijn commented Jul 1, 2021

jeffkreeftmeijer commented Jul 2, 2021

tombruijn Jul 2, 2021

Choose a reason for hiding this comment

jeffkreeftmeijer commented Jul 6, 2021

tombruijn Jul 6, 2021

Choose a reason for hiding this comment

jeffkreeftmeijer Jul 7, 2021

Choose a reason for hiding this comment

tombruijn left a comment • edited Loading

Choose a reason for hiding this comment

tombruijn commented Jul 7, 2021

jeffkreeftmeijer commented Jun 24, 2021 •

edited

Loading

tombruijn Jun 30, 2021 •

edited

Loading

jeffkreeftmeijer Jul 5, 2021 •

edited

Loading

tombruijn left a comment •

edited

Loading