-
Notifications
You must be signed in to change notification settings - Fork 5
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: adding a source map upload CLI package #102
Conversation
.github/workflows/run-tests.yml
Outdated
@@ -16,7 +16,7 @@ jobs: | |||
- name: Setup NPM | |||
uses: actions/setup-node@v3 | |||
with: | |||
node-version: 19.x | |||
node-version: 20.x |
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.
suggestion (for a separate task):
Let's test with the recent node versions like we are doing is for Faro.
We support the current active version till the oldest maintenance version.
For versions which reached ool we do not provide support.
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.
Please add tests for the important logic.
@@ -134,3 +135,93 @@ The following options are available for the Faro JavaScript bundler plugins: | |||
After initial configuration, the Faro JavaScript bundler plugins automatically uploads your source maps to Grafana Cloud when you build your application. You can verify that the source maps upload successfully by in the "Settings" -> "Source Maps" tab in the Frontend Observability plugin. From there you are able to see the source maps that you have uploaded. | |||
|
|||
After you have completed all the required steps, you have finished - the Faro Collector begins processing your source maps and associating them with your telemetry data. The portions of your stack traces with source maps uploaded to the Faro Collector are automatically de-obfuscated and displayed in the Frontend Observability plugin when viewing your error data. | |||
|
|||
## CLI for Sourcemap Uploads |
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.
Let's also add this to the cloud docs 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.
All in all LGTM. I left some comments regarding the use cURL directly from the command line and some other nitpicks.
packages/faro-cli/src/cli.ts
Outdated
options.gzipPayload | ||
); | ||
|
||
console.log(curlCommand); |
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.
nit: Maybe add a message here that this is a curl command. Although I guess the API is pretty self explanatory 🤷♂️
|
||
try { | ||
// Execute curl command to upload the file | ||
success = executeCurl( |
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.
Out of curiosity, is there a reason to using curl over the command line instead of using native http requests or axios or even bindings to libcurl?
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 had a user recently express that their CI pipeline's proxy configuration caused fetch to fail and they needed to resort to cURL. turns out there is a whole proxy agent config that we can pass along to fetch/axios and I felt like this would be simpler in the long run.
packages/faro-cli/src/index.ts
Outdated
}; | ||
|
||
/** | ||
* Uploads multiple sourcemap files compressed as a tarball to the Faro API using cURL |
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.
very nitpicky: Technically speaking tarball is not compressed but rather a gzipped tarball
@@ -26,11 +27,17 @@ export default function faroUploader( | |||
keepSourcemaps, | |||
gzipContents, | |||
verbose, | |||
skipUpload, |
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! I like this option
solving #103
what
this PR adds a package for uploading source maps via CLI and provides an option to disable uploading from the bundler plugins
why
this feature has been requested to enable the ability to decouple the upload process from the build pipeline. this should enable more use cases and provide more flexibility to those who want to upload their source maps in a more customizable way.