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
docs: improve docs around bundlers/transpilers, methods of starting the agent #2837
Conversation
Our docs on ESM usage with Babel state that you need to use 'elastic-apm-node/start' to instrument properly. Our test case should do so too.
…dd ES Module support limitation note (will have to clarify the somewhat confusing es-modules.html content)
This is experimental. I'm not sure if there are side-effects. See #1967 (comment)
…the Bundlers gotchas section
Adding this with*out* extension would be problematic. Currently without an "exports" a "foo.mjs" can `import 'elastic-apm-node/start.js'` (requires the .js) and can `import 'elastic-apm-node/lib/config.js'` et al. That's fine. No need to lock it down. If we *really* want the extension*less* access names, then we need to add "exports"... which locks it down, which *could* be breaking. So... let's not. I think we should bias to including the extension as ESM becomes more popular.
… and reference to the updated sdtarting-the-agent docs and the ES Modules section in supported-technologies. Also add the start-option-separate-init-module start option.
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.
This is looking really good! Here are some nitpicks, questions, and ideas for you to consider.
docs/set-up.asciidoc
Outdated
// Application main code goes here. | ||
---- | ||
|
||
When authoring code using CommonJS (i.e. using `require(...)`), this method is rarely used. However, when using a tool (such as Babel or esbuild) to translate/transpile from code using ES modules (i.e. `import ...` statements) to code using CommonJS, this start method is necessary to ensure that the APM agent is _started before other imports in the same file_. See <<start-esm-imports>> below for details. |
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.
The second sentence is very long (and has multiple parentheticals). Can we break it up? What about something like this?
This method is rarely used when authoring code using CommonJS (i.e. using
require(...)
).
However, this method is necessary if you use a tool like Babel or esbuild to translate/transpile from code using ES modules (i.e. usingimport ...
statements) to code using CommonJS. This start method ensures that the APM agent is started before other imports in the same file. See <> below for details.
Also, what do you mean when you say "rarely used"? Is there a use case for this method and CommonJS that we should explain?
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.
Also, what do you mean when you say "rarely used"?
There isn't a use case for this beyond saving a couple keystrokes over require('elastic-apm-node').start()
at the cost of not having a way to pass options to that .start()
call.
@bmorelli25 How about I just drop the reference to CommonJS usage completely? There is no harm if a require
-using programmer uses this form. So something like this:
This start method exists for those that use a tool like Babel or esbuild to translate/transpile from code using ES modules (as in the following example) to code using CommonJS. This start method ensures that the APM agent is started before other imports in the same file. See ... below for details.
Co-authored-by: Brandon Morelli <brandon.morelli@elastic.co>
Co-authored-by: Brandon Morelli <brandon.morelli@elastic.co>
Co-authored-by: Brandon Morelli <brandon.morelli@elastic.co>
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.
Approving. Overall it says true things and gives us a better place to point folks working with TypeScript and other compiled languages. I didn't go full tech review on all the examples but what's there seems good. Approving presuming the more written word advice/feedback from @bmorelli25 in incorporated/resolved.
@@ -41,4 +41,12 @@ This endpoint has moved. Please see <<transaction-add-labels>>. | |||
[role="exclude",id="get-started"] |
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.
TIL: we have a deleted pages page.
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.
Yeah! They're perfect for this use case.
@@ -1,7 +1,7 @@ | |||
[[set-up]] |
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.
TIL: we have a doc named set-up.asciidoc
and a doc named setup.asciidoc
.
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.
Yah, it drives me crazy. :) And an <<advanced-setup>>
anchor that is labelled "Configuration", but a separate <<configuration>>
anchor that is labelled "Configuration options". Neither of these are in the "Set up the Agent" section.
Co-authored-by: Brandon Morelli <brandon.morelli@elastic.co>
Closes: #1967
review notes
Here are some highlights of what changes this PR makes to the docs:
import apm from 'elastic-apm-node/start'
to start the agent before other imports.