-
Notifications
You must be signed in to change notification settings - Fork 53
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(telemetry): Add telemetry via @sentry/node
#213
Conversation
fd74f0a
to
00a2e24
Compare
00a2e24
to
2839b6b
Compare
src/telemetry.ts
Outdated
event.exception?.values?.forEach(exception => { | ||
delete exception.stacktrace; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this is even necessary in the action? Can it run anywhere else than on github's environment? Our main objective for deleting the stack trace in the wizard and plugins was that the stack trace file paths would likely come from users' devices.
Happy with either way, just wanted to ask if it's necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People could theoretically, for whatever reason, execute this action via something like https://github.com/nektos/act but I think the chance is rather low.
Will remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the hooks as well for the same reason.
src/main.ts
Outdated
}, | ||
async () => { | ||
try { | ||
const cli = getCLI(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l/suggestion: In the bundler plugins, we pass the trace to the CLI. Maybe worth to do this here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added here: f14edb3
590975a
to
b9f36f1
Compare
72f6dfe
to
248798c
Compare
@Lms24 I added session tracking and the |
586f67a
to
248798c
Compare
faeb117
to
93f5b5e
Compare
dca951d
to
202993b
Compare
0105914
to
202993b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
}).releases; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm: the for
loop for setting the variables was moved to checkEnvironmentVariables
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, correct. That was really only necessary once at startup and since getCli
is now not returning a singleton anymore I moved it over there.
Adds telemetry by setting up a barebones Sentry instance using
@sentry/node
. Individual steps can be traced by wrapping them with thetraceStep
helper.Here's an example of an execution:
Closes: #214