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

fix(Telemetry): Ensure telemetry only matches js stacktrace paths #9447

Merged
merged 1 commit into from May 10, 2021

Conversation

pgrzesik
Copy link
Contributor

Reported internally

It should resolve situations where the regex was not strict enough and was matching phrases as

  • from versions: none (from Python requirements plugin)
  • unix:///var/run/docker.sock. Is the docker daemon running?. (from Docker check)
  • does not match the API schema. (from webpack)

Additionally, it will not report the stacktrace lines coming from internals such as node:child_process:573:9' which are noise anyway.

@codecov
Copy link

codecov bot commented May 10, 2021

Codecov Report

Merging #9447 (3bf20b2) into master (8d56d0e) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 3bf20b2 differs from pull request most recent head a176c8a. Consider uploading reports for the commit a176c8a to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9447   +/-   ##
=======================================
  Coverage   86.90%   86.90%           
=======================================
  Files         321      321           
  Lines       11976    11976           
=======================================
  Hits        10408    10408           
  Misses       1568     1568           
Impacted Files Coverage Δ
lib/utils/telemetry/resolve-error-location.js 100.00% <100.00%> (ø)
lib/cli/parse-args.js 100.00% <0.00%> (ø)
scripts/serverless.js 50.00% <0.00%> (ø)
lib/classes/Variables.js 99.73% <0.00%> (ø)
lib/plugins/aws/deployFunction.js 96.62% <0.00%> (ø)
lib/cli/ensure-supported-command.js 94.28% <0.00%> (ø)
lib/plugins/aws/invokeLocal/index.js 68.87% <0.00%> (ø)
lib/plugins/aws/customResources/index.js 98.57% <0.00%> (ø)
lib/plugins/aws/package/compile/layers.js 84.61% <0.00%> (ø)
lib/plugins/aws/package/compile/functions.js 95.28% <0.00%> (ø)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d56d0e...a176c8a. Read the comment docs.

@pgrzesik pgrzesik self-assigned this May 10, 2021
@pgrzesik pgrzesik requested a review from medikoo May 10, 2021 10:21
@pgrzesik pgrzesik force-pushed the fix-filtering-for-stacktrace-line-regex branch from 5fca5ac to 9b4e778 Compare May 10, 2021 11:13
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Wouldn't it be better if would confirm it's a stack trace line by requiring: :\d+:\d+?

e.g. I think it's valuable to have node:internal/child_process:403:11 reported, as having Node.js version, we can seclude which exactly

@@ -8,7 +8,7 @@ const resolveErrorLocation = (exceptionTokens) => {
const splittedStack = exceptionTokens.stack.split(/[\r\n]+/);
if (splittedStack.length === 1 && exceptionTokens.code) return '<not available>';

const stacktraceLineRegex = /(?:\s*at.*\((.*)\).?|\s*at\s(.*))/;
const stacktraceLineRegex = /(?:\s*at.*\((.*\.js:\d+:\d+)\).?|\s*at\s(.*\.js:\d+:\d+))/;
Copy link
Contributor

Choose a reason for hiding this comment

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

It may stop working once someone starts using e.g. mjs or cjs extension.

I think problem with regex was that it didn't force whitespace wrap around at word.

How about a regex as /(?:\s+at\s+\((.*:\d+:\d+)\).?|\s+at\s+(.*:\d+:\d+))/;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that was the problem with the regex and in the initial version I went with the exact same regex as you're proposing, but I thought that maybe we could be more "bulletproof" with enforcing js, but your point about mjs and cjs makes a lot of sense as well, I'll stick to just looking for line/column numbers at the end for now and will monitor if it resolves the problem 👍

@pgrzesik pgrzesik force-pushed the fix-filtering-for-stacktrace-line-regex branch from 9b4e778 to a176c8a Compare May 10, 2021 13:41
@pgrzesik pgrzesik requested a review from medikoo May 10, 2021 13:47
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

👍

@pgrzesik pgrzesik merged commit 7361e04 into master May 10, 2021
@pgrzesik pgrzesik deleted the fix-filtering-for-stacktrace-line-regex branch May 10, 2021 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants