fix(node): Only set DeviceContext.boot_time if os.uptime() is valid#5859
fix(node): Only set DeviceContext.boot_time if os.uptime() is valid#5859
DeviceContext.boot_time if os.uptime() is valid#5859Conversation
AbhiPrasad
left a comment
There was a problem hiding this comment.
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.
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 |
|
We might also want to wrap more of this code in 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 |
|
Good Point, Tim. I did a little digging and I think for the azure use case we're probably good because I think though, the try/catch blog seems like a good suggestion because who knows what other runtimes permit/return. |
When creating the
DeviceContextin the Node SDKContextintegration, we calculate the boot time based onos.uptime. However, as reported in #5856,os.uptimemight 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
0for 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 @timfishfixes #5856