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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(node): Only set DeviceContext.boot_time if os.uptime() is valid #5859

Merged
merged 3 commits into from Sep 30, 2022

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

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鈥檚 approve to unblock, maybe even cut a patch for it (I鈥檒l let you make the judgement call there), but weird that this happens, let me do some research on my end as well.

packages/node/src/integrations/context.ts Outdated Show resolved Hide resolved
@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)
3 participants