Skip to content
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

fix(duration accuracy for coldstarts by using process.uptime) #292

Closed
wants to merge 1 commit into from

Conversation

ewindisch
Copy link
Contributor

Fixes #283

Signed-off-by: Erica Windisch 73207+ewindisch@users.noreply.github.com

Signed-off-by: Erica Windisch <73207+ewindisch@users.noreply.github.com>
@ewindisch ewindisch changed the title Use process uptime for duration when coldstart detected fix(duration accuracy for coldstarts by using process.uptime) Sep 1, 2018
@pselle
Copy link
Contributor

pselle commented Sep 4, 2018

@ewindisch re-ran the workflow and tests are green now -- if your commit is formatted in the semantic release formatting (you can do this by running yarn commit when you commit), it'll trigger a release. Request a review when you want, LGTM :)

Copy link
Contributor

@kolanos kolanos left a comment

Choose a reason for hiding this comment

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

According to the node docs:
https://nodejs.org/api/process.html#process_process_uptime

process.uptime() is the time since the process was started, the duration is meant to measure invocation time. Since a process can encompass many invocations and the process gets frozen in periods of inactivity, I'm not sure this accomplishes the goal of making the duration more accurate. Maybe there's a way to store the uptime before the process is frozen ad then compare the current uptime against that value for each invocation?

@@ -212,6 +212,7 @@ class Report {

if (this.report.coldstart) {
this.report.labels.add('@iopipe/coldstart');
this.report.duration = Math.ceil(process.uptime() * 1e9);
Copy link
Contributor

Choose a reason for hiding this comment

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

@ewindisch Isn't this the uptime since the process was created? Wouldn't that encompass many invocations?

@pselle
Copy link
Contributor

pselle commented Nov 15, 2018

Closing as stale

@pselle pselle closed this Nov 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants