Skip to content

Conversation

@Lms24
Copy link
Member

@Lms24 Lms24 commented Sep 30, 2022

When creating the DeviceContext in the Node SDK Context integration, we calculate the boot time based on os.uptime. However, as reported in #5856, os.uptime might not always be available or return undefined (I couldn't figure out which of the two cases actually was responsible).

This PR quickly fixes this bug by simply not setting the boot time in case we don't get a valid up time. It also adds two basic tests for this behaviour. I opted to not set the boot time instead of e.g. defaulting to 0 for uptime because IMO this creates a false boot time that probably causes more confusion than not having it. However, as my context on the Context integration (👀) is limited, I'm interested in other opinions on this. cc @timfish

fixes #5856

@Lms24 Lms24 requested review from AbhiPrasad and timfish September 30, 2022 08:43
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Let’s approve to unblock, maybe even cut a patch for it (I’ll let you make the judgement call there), but weird that this happens, let me do some research on my end as well.

@Lms24
Copy link
Member Author

Lms24 commented Sep 30, 2022

maybe even cut a patch for it

While I don't think this has super high prio, I think there's nothing holding us back from releasing a patch. Will coordinate with the rest of the team

@Lms24 Lms24 merged commit 92dd0ed into master Sep 30, 2022
@Lms24 Lms24 deleted the lms-fix-azure-device-context-boottime branch September 30, 2022 09:44
@timfish
Copy link
Collaborator

timfish commented Sep 30, 2022

We might also want to wrap more of this code in try/catch Just In Case™️.

If we were only dealing with vanilla Node.js we can rely heavily on the types + tests but with all these node/v8 derived/patched runtimes it feels like a lot of guesswork!

The user report shows it failing on os.uptime but with that fixed it might then fail on the CPU stuff...

@Lms24
Copy link
Member Author

Lms24 commented Sep 30, 2022

Good Point, Tim. I did a little digging and I think for the azure use case we're probably good because os.uptime seems to be the only thing that returns undefined. Left a small comment about this in the bug report issue.

I think though, the try/catch blog seems like a good suggestion because who knows what other runtimes permit/return.

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.

7.x causes invalid range error (process.uptime)

4 participants