Skip to content

Conversation

basepi
Copy link
Contributor

@basepi basepi commented Jul 13, 2021

Remaining:

Related issues

Ref #428

@basepi basepi self-assigned this Jul 13, 2021
@ghost
Copy link

ghost commented Jul 13, 2021

💚 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: 2021-10-18T14:57:40.848+0000

  • Duration: 29 min 14 sec

  • Commit: 744a39e

Test stats 🧪

Test Results
Failed 0
Passed 10127
Skipped 8977
Total 19104

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

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

  • /test : Re-trigger the build.
basepi added 3 commits July 13, 2021 11:40
* Add disable_metrics_thread conf * Only use the transport background thread * Singleton client for the whole lambda file * Flush after transaction
@AlexanderWert AlexanderWert linked an issue Jul 15, 2021 that may be closed by this pull request
Copy link
Contributor

@beniwohli beniwohli left a comment

Choose a reason for hiding this comment

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

Regarding time_to_perf_counter, it generally LGTM. I worry however that especially in an environment like Lambda, where the runtime is put to sleep and woken up on a regular basis, clock skew could be a phenomenon that is more regular than usual. But that's just a hunch...

@basepi
Copy link
Contributor Author

basepi commented Jul 20, 2021

Regarding time_to_perf_counter, it generally LGTM. I worry however that especially in an environment like Lambda, where the runtime is put to sleep and woken up on a regular basis, clock skew could be a phenomenon that is more regular than usual. But that's just a hunch...

I don't think that's actually an issue in this case. perf_counter is designed to be monotonic and take sleep into account. time.time() is designed to be "real-world" (take sleep into account) the same as perf_counter, and will be monotonic absent a system clock change. I can't imagine we will have to deal with system clock changes in AWS but if we do, we might have a weird outlier but there's not much we can do about that.

Edit: I am going to do some testing around perf_counter and the freezing that lambda does. I think it should still work, but if it doesn't we definitely want to make sure we take these measurements each invocation.

@basepi
Copy link
Contributor Author

basepi commented Jul 20, 2021

Did some testing today and the conversion measurement we take at the beginning of the cold start appears to stay accurate throughout multiple invocations, with freezes in between.

I'm still going to implement some re-measurement for long-running processes but I don't think we need to measure every time with Lambda like I was fearing.

@AlexanderWert AlexanderWert added this to the 7.16 milestone Aug 25, 2021
@basepi basepi requested a review from beniwohli October 14, 2021 17:02
@basepi basepi marked this pull request as ready for review October 14, 2021 17:02
Copy link
Contributor

@beniwohli beniwohli left a comment

Choose a reason for hiding this comment

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

Great work, looks good!

I think we need a better solution for passing state along in the capture_serverless object. Especially when used as a module-level decorator, the chance of things going awry are pretty big IMO.

@basepi basepi merged commit ec47874 into elastic:master Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants