Skip to content

Conversation

AlexanderWert
Copy link
Member

@AlexanderWert AlexanderWert commented Feb 8, 2022

@github-actions github-actions bot added the aws-λ-extension AWS Lambda Extension label Feb 8, 2022
@ghost
Copy link

ghost commented Feb 8, 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-02-14T08:10:16.802+0000

  • Duration: 12 min 7 sec

Test stats 🧪

Test Results
Failed 0
Passed 84
Skipped 6
Total 90

🤖 GitHub comments

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

  • /test : Re-trigger the build.

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

@AlexanderWert
Copy link
Member Author

AlexanderWert commented Feb 8, 2022

@trentm, @astorm, @basepi With this update I also removed the docs on requiring to set the following config options (as I think they rather belong to the individual agents, e.g. for Java they don't need to be set):

  • set ELASTIC_APM_SERVER_URL to http://localhost:8200
  • set ELASTIC_APM_CLOUD_PROVIDER to none
  • set ELASTIC_APM_CENTRAL_CONFIG to false

Question: Are the above config options set by the Node.js and Python agents anyways when the agents detect that they are running on Lambda? Or do we need to adapt the agent-specific lambda docs to include those config options there again?

@basepi
Copy link
Contributor

basepi commented Feb 8, 2022

Are the above config options set by the Node.js and Python agents anyways when the agents detect that they are running on Lambda? Or do we need to adapt the agent-specific lambda docs to include those config options there again?

https://github.com/elastic/apm-agent-python/blob/ec9a733d6632e6a2aacafe2008a019724f062aa2/elasticapm/contrib/serverless/aws.py#L76-L78

We set ELASTIC_APM_CLOUD_PROVIDER and ELASTIC_APM_CENTRAL_CONFIG, but not ELASTIC_APM_SERVER_URL.

Adding the latter is easy if we are just setting it to localhost. If we're trying to automatically convert the value of ELASTIC_APM_SERVER_URL to ELASTIC_APM_LAMBDA_APM_SERVER it's a little more difficult, but doable.

Are we targeting that automatic conversion, or do we require the users to set ELASTIC_APM_LAMBDA_APM_SERVER?

@AlexanderWert
Copy link
Member Author

@basepi

Are we targeting that automatic conversion, or do we require the users to set ELASTIC_APM_LAMBDA_APM_SERVER?

We don't target that automatic conversion. We expect the users to set this env var for the extension. But in this case it would be less confusing for the users if we would set ELASTIC_APM_SERVER_URL to http://localhost:8200 internally (without them needing to care about that value) in the agents when we detect that the agent runs within an AWS lambda environment.

I guess we also need to update the lambda spec to include this.

@AlexanderWert
Copy link
Member Author

Just checked, http://localhost:8200 is the default value for ELASTIC_APM_LAMBDA_APM_SERVER for all the agents, anyhow. @basepi I guess we don't need to explicitly set this in case of lambda. So, we should be fine with the current state now.

@trentm
Copy link
Member

trentm commented Feb 8, 2022

Just checked, http://localhost:8200 is the default value for ELASTIC_APM_LAMBDA_APM_SERVER for all the agents, anyhow.

@AlexanderWert You mean ELASTIC_APM_SERVER here, right? The agents don't need to know anything about the ELASTIC_APM_LAMBDA_APM_SERVER var.

  • set ELASTIC_APM_SERVER_URL to http://localhost:8200

  • set ELASTIC_APM_CLOUD_PROVIDER to none

  • set ELASTIC_APM_CENTRAL_CONFIG to false

Question: Are the above config options set by the Node.js and Python agents anyways when the agents detect that they are running on Lambda? Or do we need to adapt the agent-specific lambda docs to include those config options there again?

The Node.js agent currently does not automatically set any of these when in a lambda environment. It could easily do so, and we could get a release out with them. Shall I open a ticket for this?

We might want our Lambda docs to specifically mention a minimum APM agent version to use for each of the languages. Given we are still "experimental" we could perhaps get away without bothering at this point?

@AlexanderWert
Copy link
Member Author

@trentm

@AlexanderWert You mean ELASTIC_APM_SERVER here, right? The agents don't need to know anything about the ELASTIC_APM_LAMBDA_APM_SERVER var.

Yes, you're right, this was a typo.

The Node.js agent currently does not automatically set any of these when in a lambda environment.

OK. But the default value for ELASTIC_APM_SERVER_URL is http://localhost:8200 according to the docs, so no need to set this explicitly, right?

It could easily do so, and we could get a release out with them. Shall I open a ticket for this?

I think this would simplify the onboarding experience for the users. And if it's a small change then let's do it as part of elastic/apm-agent-nodejs#2531.

We might want our Lambda docs to specifically mention a minimum APM agent version to use for each of the languages. Given we are still "experimental" we could perhaps get away without bothering at this point?

Like that proposal!

@AlexanderWert AlexanderWert marked this pull request as ready for review February 9, 2022 07:41
Copy link
Contributor

@estolfo estolfo left a comment

Choose a reason for hiding this comment

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

just two very minor comments

@trentm
Copy link
Member

trentm commented Feb 9, 2022

OK. But the default value for ELASTIC_APM_SERVER_URL is http://localhost:8200 according to the docs, so no need to set this explicitly, right?

Correct.

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.

Looks great overall and the ARNs will greatly simplify things for users. Huzzah.

Two quick things that could this even better

  1. If possible let's fix the docs build error so we're not scratching our heads come release time.

  2. I notice that we've removed all mention of the installer -- this is mostly OK as the installer is documented in its own README.md. However, by removing the installer docs we also no longer have a section where we provide a list of the required/recommended env variables that a user could copy/paste. Also -- the installer smoothed over the whole "ELASTIC_APM_SERVER_URL doesn't point to APM Server, it points to the extension and ELASTIC_APM_LAMBDA_APM_SERVER is what points to the actual APM server" thing.

What do you think about adding a code section that lists the default/recommended env variables in one place? Something with a copy/pasteable code section that looks something like the following

ELASTIC_APM_CENTRAL_CONFIG=false
ELASTIC_APM_CLOUD_PROVIDER=none
ELASTIC_APM_DATA_RECEIVER_TIMEOUT_SECONDS=15
ELASTIC_APM_LAMBDA_APM_SERVER=https://your-apm-server-url:443 # this is your _real_ APM Server URL
ELASTIC_APM_SECRET_TOKEN=shhhhhhitsasecret
ELASTIC_APM_SERVER_URL=http://localhost:8200  # this points at the extension endpoint

@AlexanderWert
Copy link
Member Author

@astorm

I notice that we've removed all mention of the installer -- this is mostly OK as the installer is documented in its own README.md. However, by removing the installer docs we also no longer have a section where we provide a list of the required/recommended env variables that a user could copy/paste. Also -- the installer smoothed over the whole "ELASTIC_APM_SERVER_URL doesn't point to APM Server, it points to the extension and ELASTIC_APM_LAMBDA_APM_SERVER is what points to the actual APM server" thing.

The following three options will be set to meaningful defaults by the APM agents internally (when they detect an AWS Lambda environment):

  • ELASTIC_APM_CENTRAL_CONFIG
  • ELASTIC_APM_CLOUD_PROVIDER
  • ELASTIC_APM_SERVER_URL

So, to reduce confusion, I think we should not even mention them here. WDYT?

This leaves us with the documented, extension-specific config options.

Do you think having a code-block (like the one below) in the docs for these options in addition would provide additional value?

ELASTIC_APM_LAMBDA_APM_SERVER=https://your-apm-server-url:443 # this is your _real_ APM Server URL
ELASTIC_APM_SECRET_TOKEN=shhhhhhitsasecret
ELASTIC_APM_SERVICE_NAME=myApmServiceName # (optional) use this to group multiple lambda functions
ELASTIC_APM_DATA_RECEIVER_TIMEOUT_SECONDS=15 # (optional) this is the default falue
ELASTIC_APM_SEND_STRATEGY=syncflush # (optional) this is the default strategy

@astorm
Copy link
Contributor

astorm commented Feb 11, 2022

@AlexanderWert

The following three options will be set to meaningful defaults by the APM agents internally (when they detect an AWS Lambda environment) ... So, to reduce confusion, I think we should not even mention them here. WDYT?

WITI: If that behavior can be successfully coordinated across the agents then yes, it doesn't seem like we'll need to mention these specific variables.

This leaves us with the documented, extension-specific config options.
Do you think having a code-block (like the one below) in the docs for these options in addition would provide additional value?

Good question -- thinking out loud, without one central place in the docs that shows the required lambda env. configuration how will a new user/customer know what values are required when setting up their new Lambda function? Or put another way, is is acceptable to presume that they'll read each individual env. var definition and extract a working configuration themselves? @bmorelli25 Do you have and advice/insight on how we tend to handle these sorts of configuration heavy situations across our docs? Is it better or worse to hand the user a full block of the default configuration values they'll need?

All that said -- my opinions here aren't strong enough to be blocking. I just want to make sure we're being intentional. :) Approving as is and leaving the above in your hands.

@trentm
Copy link
Member

trentm commented Feb 11, 2022

WITI

?
https://acronyms.thefreedictionary.com/WITI is failing me here.

@bmorelli25
Copy link
Member

@bmorelli25 Do you have and advice/insight on how we tend to handle these sorts of configuration heavy situations across our docs? Is it better or worse to hand the user a full block of the default configuration values they'll need?

At least in the APM Server docs, we do typically provide a copy/pastable code block of configs to show what a typical or basic setup might look like.

For the settings with meaningful defaults, I think it's fine to omit them from the documentation for now. If an SDH or Discuss question comes in, we can reconsider.

WITI

When I think about it? Might be missing an "a". Fun game.

@trentm
Copy link
Member

trentm commented Feb 11, 2022

Wordle Is Totally Intractable?

@AlexanderWert AlexanderWert merged commit 86a442c into elastic:main Feb 14, 2022
bmorelli25 pushed a commit to bmorelli25/apm-aws-lambda that referenced this pull request Mar 1, 2022
…lastic#122)

* Update aws-lambda-extension.asciidoc describing the simplified onboarding for the extension.

* Update docs/aws-lambda-extension.asciidoc

Co-authored-by: Trent Mick <[email protected]>

* Update docs/aws-lambda-extension.asciidoc

Co-authored-by: Emily S <[email protected]>

* Update docs/aws-lambda-extension.asciidoc

Co-authored-by: Emily S <[email protected]>

* Update docs/aws-lambda-extension.asciidoc

Co-authored-by: Trent Mick <[email protected]>

* Update aws-lambda-extension.asciidoc

* added example configuration for the extension

* fix reference

Co-authored-by: Trent Mick <[email protected]>
Co-authored-by: Emily S <[email protected]>
bmorelli25 added a commit that referenced this pull request Mar 1, 2022
…122) (#134)

* Update aws-lambda-extension.asciidoc describing the simplified onboarding for the extension.

* Update docs/aws-lambda-extension.asciidoc

Co-authored-by: Trent Mick <[email protected]>

* Update docs/aws-lambda-extension.asciidoc

Co-authored-by: Emily S <[email protected]>

* Update docs/aws-lambda-extension.asciidoc

Co-authored-by: Emily S <[email protected]>

* Update docs/aws-lambda-extension.asciidoc

Co-authored-by: Trent Mick <[email protected]>

* Update aws-lambda-extension.asciidoc

* added example configuration for the extension

* fix reference

Co-authored-by: Trent Mick <[email protected]>
Co-authored-by: Emily S <[email protected]>

Co-authored-by: Alexander Wert <[email protected]>
Co-authored-by: Trent Mick <[email protected]>
Co-authored-by: Emily S <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aws-λ-extension AWS Lambda Extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants