feat(compass-shell): integrate shell logging COMPASS-5035 #2458
Conversation
| for (let i = 0; i < log.length; i++) { | ||
| const entry = log[i]; | ||
| expect(entry.t.$date).to.be.a('string'); | ||
| delete entry.t; // Timestamps vary between each execution |
There was a problem hiding this comment.
Unrelated, but it looks like this function modifies the passed in log - was that intentional? I changed the behavior in #2438 but wasn't sure if it was for a reason I missed.
There was a problem hiding this comment.
It’s taking a copy of the log right at the beginning anyway, so yeah, this was intentional… we can see whichever PR gets merged first and then adjust the other
| platform: process.platform, | ||
| arch: process.arch, | ||
| }, | ||
| require('../../package.json').version |
There was a problem hiding this comment.
I don't think we need to do anything now, but on the mongosh logger side of this the variable received here is called mongosh_version which might be a bit misleading in telemetry since it's really the compass-shell version that we'll be showing here. Would it make sense to have both the consumer (compass-shell) version and the mongosh version here?
https://github.com/mongodb-js/mongosh/blob/main/packages/logging/src/setup-logger-and-telemetry.ts#L82
There was a problem hiding this comment.
So … this argument is only used for telemetry (which we don’t use here), not for logging, so I was thinking that it’s fine to just use the next best thing.
The problem is that the only consistent “mongosh” version that we definitely have is the CLI package’s version, which may also just end up not applying anymore here if we ever decide to use lerna independent mode in mongosh. I’ve added logging of the service-provider-server package version on the mongosh side, right now that’s in sync with the CLI package, but I’ve also warned Felicia that they might diverge at some point. :)
| // Remove server heartbeat logs as they can happen a varying amount of | ||
| // times depending on how long tests are taking. | ||
| if (entry.id === 1_001_000_022 || entry.id === 1_001_000_023) { | ||
| log.splice(i--, 1); |
There was a problem hiding this comment.
How do you feel about adding a continue here just to be intentional about moving to the next entry? Might prevent future bugs too if more cases to skip entries are added which could be non-exclusive to this or the case below.
There was a problem hiding this comment.
Will include that in the next PR :)
3 participants
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes