Skip to content

feat: Make Agent.flush() return a Promise if no callback is passed as param - #3167

Merged
trentm merged 8 commits into
elastic:mainfrom
adamyeats:awaitable-flush
Feb 25, 2023
Merged

feat: Make Agent.flush() return a Promise if no callback is passed as param#3167
trentm merged 8 commits into
elastic:mainfrom
adamyeats:awaitable-flush

Conversation

@adamyeats

@adamyeats adamyeats commented Feb 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #2857.

This PR modifies Agent.flush() to return a Promise if no callback parameter is supplied to the function. This allows the method to be usable with the await keyword.

Checklist

  • Implement code
  • Add tests
  • Update TypeScript typings
  • Update documentation
  • Add CHANGELOG.asciidoc entry
  • Commit message follows commit guidelines

@cla-checker-service

cla-checker-service Bot commented Feb 24, 2023 •
edited
Loading

Copy link
Copy Markdown

💚 CLA has been signed

@github-actions github-actions Bot added agent-nodejs Make available for APM Agents project planning. community triage labels Feb 24, 2023
@ghost

ghost commented Feb 24, 2023 •
edited by ghost
Loading

Copy link
Copy Markdown

💚 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 previewSnapshots

Expand to view the summary

Build stats

  • Start Time: 2023-02-24T23:57:56.066+0000

  • Duration: 24 min 38 sec

🤖 GitHub comments

Expand to view the 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

trentm commented Feb 24, 2023

Copy link
Copy Markdown
Member

Hi @adamyeats. Nice. This is still "draft", so I'm not sure if you wanted review on it yet. The implementation looks perfect to me.

  1. Would you be willing to add a test? A test(...) in this block should suffice: https://github.com/elastic/apm-agent-nodejs/blob/main/test/agent.test.js#L932 That test file can be run directly via node test/agent.test.js.
  2. To pass the current (semicolon-hostile) style (npm run lint:eslint, aka make check) you'll need to drop the semicolons.

@trentm trentm self-assigned this Feb 24, 2023
@trentm trentm removed the triage label Feb 24, 2023
@adamyeats

adamyeats commented Feb 24, 2023

Copy link
Copy Markdown
Contributor Author

Hey @trentm, great to make contact with you! 👋 As you noticed, I haven't added tests yet thus it was still in draft, but thank you for the review on what was already there.

  • Test is incoming for this, I'll add something ASAP!
  • Thanks for picking up on the formatting, looks like ESlint integration was broken in my copy of VS Code. This should be resolved in 5776936.

Quick question: is there anywhere in the documentation that will need to be edited? If so, can you point me in the right direction?

@trentm

trentm commented Feb 24, 2023

Copy link
Copy Markdown
Member

is there anywhere in the documentation that will need to be edited?

Yah, look for "apm.flush" in https://github.com/elastic/apm-agent-nodejs/blob/main/docs/agent-api.asciidoc
Thanks.

@adamyeats
adamyeats marked this pull request as ready for review February 24, 2023 23:44
@adamyeats

adamyeats commented Feb 24, 2023

Copy link
Copy Markdown
Contributor Author

@trentm This should now be ready for review, thanks!

trentm
trentm approved these changes Feb 24, 2023
View reviewed changes

@trentm trentm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@adamyeats This is great, thanks! One nit, and one thing in the test. I'm guessing the test with node v8 will fail. We shall see. I'm "approving" because, IIRC, our Jenkins CI guards require an approval before running contributed code.

Comment thread CHANGELOG.asciidoc Outdated
Comment thread test/agent.test.js Outdated
Comment on lines +1160 to +1163
}).finally(function () {
agent.destroy()
t.end()
})

@trentm trentm Feb 24, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unfortunately we still support and test with node v8, which doesn't support Promise.finally. I think it would be fine to put the agent.destroy(); t.end() in the .then(...) handler and call it a day.

@adamyeats adamyeats Feb 25, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, those were the days! Agreed, tweaked this in 6a6afea.

adamyeats and others added 2 commits February 25, 2023 00:55
@trentm

trentm commented Feb 25, 2023

Copy link
Copy Markdown
Member

@elasticmachine, run elasticsearch-ci/docs

@trentm
trentm merged commit 40e355b into elastic:main Feb 25, 2023
@trentm

trentm commented Feb 25, 2023

Copy link
Copy Markdown
Member

Parfait. Thanks very much!

@adamyeats
adamyeats deleted the awaitable-flush branch February 25, 2023 01:06
This was referenced Feb 27, 2023
Open
Closed
v1v added a commit to v1v/apm-agent-nodejs that referenced this pull request Mar 21, 2023
@v1v
Merge remote-tracking branch 'upstream/main' into feature/action-tav
2fa46dc 
* upstream/main: (44 commits)
  action: abort builds when new commit (elastic#3196)
  docs: note that 3.40.0 was bad release (elastic#3194)
  chore(deps-dev): bump @babel/cli from 7.20.7 to 7.21.0 (elastic#3170)
  chore(deps-dev): bump @babel/core from 7.20.2 to 7.21.0 (elastic#3169)
  ci: drop max-parallel for GH actions (elastic#3191)
  test: correct sense of test message (elastic#3186)
  3.43.0 (elastic#3184)
  test, ci: some small changes (elastic#3183)
  ci: limit parallel GH Action test runs to try to avoid 429 errors on checkout (elastic#3180)
  fix: transaction name for Next.js API routes in next@13.2 was broken (elastic#3178)
  mongodb@5 support (elastic#3177)
  chore(deps-dev): bump body-parser from 1.20.1 to 1.20.2 (elastic#3171)
  chore(deps-dev): bump restify from 11.0.0 to 11.1.0 (elastic#3172)
  docs: minor fix in README for Azure Functions example (elastic#3175)
  feat: Make Agent.flush() return a Promise if no callback is passed as param (elastic#3167)
  chore(deps-dev): bump @types/node from 18.11.9 to 18.14.0 (elastic#3165)
  chore(deps-dev): bump @hapi/hapi from 21.2.1 to 21.3.0 (elastic#3166)
  chore(deps-dev): bump undici from 5.19.1 to 5.20.0 (elastic#3164)
  chore(deps-dev): bump fastify from 4.12.0 to 4.13.0 (elastic#3154)
  Use composite action for updatecli workflow (elastic#3162)
  ...
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
@adamyeats @PeterEinberger
feat: Make Agent.flush() return a Promise if no callback is passed as…
8b7b665 
… param (elastic#3167)

This PR modifies `Agent.flush()` to return a `Promise` if no `callback` parameter is supplied to the function. This allows the method to be usable with the `await` keyword.

Closes elastic#2857
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trentm trentm trentm approved these changes

Assignees

@trentm trentm

Labels

agent-nodejs Make available for APM Agents project planning. community

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

awaitable apm.flush()

2 participants

@adamyeats @trentm