Skip to content

Conversation

trentm
Copy link
Member

@trentm trentm commented Mar 31, 2022

This updates the Lambda getting started docs to use some shared widgets
from the apm-aws-lambda repo docs with the goal of making setup faster
and clearer.

Closes: #2615

This is related to and depends on elastic/apm-aws-lambda#158
See also equivalent PRs for Python (elastic/apm-agent-python#1511) and Java (elastic/apm-agent-java#2556).

Getting this to build locally to sanity test it is a bit of a chore:

mkdir lambdocs
cd lambdocs
export LAMBDOCS=`pwd`

git clone [email protected]:elastic/apm-aws-lambda.git
(cd apm-aws-lambda; git checkout 8.1)

git clone [email protected]:elastic/docs.git

git clone [email protected]:elastic/apm-agent-nodejs.git
cd apm-agent-node
git checkout trentm/improve-lambda-docs
$LAMBDOCS/docs/build_docs --doc $LAMBDOCS/apm-agent-nodejs/docs/index.asciidoc --chunk 1 --resource $LAMBDOCS/apm-aws-lambda/docs --open

Checklist

  • Update documentation

This updates the Lambda getting started docs to use some shared widgets
from the apm-aws-lambda repo docs with the goal of making setup faster
and clearer.

Closes: #2615
@trentm trentm self-assigned this Mar 31, 2022
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Mar 31, 2022
@ghost
Copy link

ghost commented Mar 31, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-03-31T23:05:54.308+0000

  • Duration: 21 min 49 sec

Test stats 🧪

Test Results
Failed 0
Passed 242815
Skipped 0
Total 242815

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules

  • run benchmark tests : Run the benchmark test only.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@trentm
Copy link
Member Author

trentm commented Mar 31, 2022

There is a docs preview build at the "docs: preview" badge link above: https://apm-agent-nodejs_2630.docs-preview.app.elstc.co/diff
Specifically this is the main changed page: https://apm-agent-nodejs_2630.docs-preview.app.elstc.co/guide/en/apm/agent/nodejs/master/lambda.html

Copy link
Member

@bmorelli25 bmorelli25 left a comment

Choose a reason for hiding this comment

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

I wonder if you also want to update the doc build script in this PR? For example, the build command in this file will need to include the new --resource $WHATEVER/apm-aws-lambda/docs that you used to build these docs.

Copy link
Contributor

@astorm astorm left a comment

Choose a reason for hiding this comment

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

Approving -- some small optional punctuation suggestions and then a few questions about the widget, but overall it looks sound and no objections to it going in.

@@ -0,0 +1,99 @@
++++
Copy link
Contributor

Choose a reason for hiding this comment

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

@trentm @bmorelli25 First -- this widget looks super cool. I didn't realize this sort of thing was possible with ASCII Doc. 💯

Second -- A couple of questions about the widget (apologies if these are retreads of discussions had elsewhere) --

  1. Is this using an authenticated or unauthenticated request to api.github.com? It looks like the later to me -- if so, do we have any concerns about hitting GitHub's API limits for public requests? Per the docs -- For unauthenticated requests, the rate limit allows for up to 60 requests per hour. Unauthenticated requests are associated with the originating IP address, and not the person making requests.?

  2. If there anything special we need to do/maintain in order to make sure this continues to work with future releases? (It looks like all we need to do is make sure the full ARNs are posted to the releases tab in GitHub as part of the release body text -- is there more to it than that?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't realize this sort of thing was possible with ASCII Doc.

All credit to @AlexanderWert and Brandon.

do we have any concerns about hitting GitHub's API limits for public requests?

It'll be limited by the IP of the personal reading the docs... so not super concerned. However, yah, I think the templating could perhaps improve to have reasonable content for if/when that request fails. Currently it'll look a little confusing. I opened elastic/apm-aws-lambda#171 for this.

anything special we need to do/maintain in order to make sure this continues to work with future releases?

My understanding is just that: full ARNs in the releases content.

Copy link
Member

Choose a reason for hiding this comment

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

All credit to @AlexanderWert and Brandon.

The ARN generator was all Alex—a really neat use of Asciidoc's HTML passthrough blocks.

@bmorelli25
Copy link
Member

bmorelli25 commented Mar 31, 2022

16:03:51 INFO:build_docs:asciidoctor: WARNING: lambda/configure-lambda.asciidoc: line 18: tag 'nodejs-env-vars' not found in include file: /tmp/docsbuild/wxbirJx9Zc/apm-aws-lambda/docs/images/images.asciidoc

I'm guessing the image hasn't been cherrypicked to 8.1 yet. Looking and fixing now.

EDIT: Fixed in elastic/apm-aws-lambda#172.

@trentm
Copy link
Member Author

trentm commented Mar 31, 2022

I wonder if you also want to update the doc build script in this PR? For example, the build command in this file will need to include the new --resource $WHATEVER/apm-aws-lambda/docs that you used to build these docs.

Done in commit c009820. Thanks for reminding me!

@trentm
Copy link
Member Author

trentm commented Mar 31, 2022

[@bmorelli25]

I'm guessing the image hasn't been cherrypicked to 8.1 yet.

Ah, no it hasn't. Will you move the pointer/reference (where ever it is) over to point to the "main" branch of apm-aws-lambda at some point?

@bmorelli25
Copy link
Member

bmorelli25 commented Mar 31, 2022

Will you move the pointer/reference (where ever it is) over to point to the "main" branch of apm-aws-lambda at some point?

@trentm Correct. As soon as all three APM Agent PRs are merged, I'll revert elastic/docs#2408. Without this mapping, main in the agent repos will revert to pointing to main in the apm-aws-lambda repo.

@bmorelli25
Copy link
Member

@elasticmachine, run elasticsearch-ci/docs

@trentm
Copy link
Member Author

trentm commented Apr 1, 2022

Merging is blocked
The base branch requires all commits to be signed. Learn more about signing commits.

Colton warned about this. I need to re-learn what setting we changed that caused this. In this case it is my commits that are not signed (bad me for not having setup GPG with GitHub). However, in the general case, we can't require external contributors to be signing commits for us to merge their PRs.

@trentm trentm merged commit ad4da62 into main Apr 1, 2022
@trentm trentm deleted the trentm/improve-lambda-docs branch April 1, 2022 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-nodejs Make available for APM Agents project planning.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

update Lambda instrumentation docs to mention the Node.js APM agent layer

3 participants