Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/compass-e2e-tests/tests/logging.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,11 @@ describe('Logging and Telemetry integration', function () {
c: 'COMPASS-DATA-SERVICE',
id: 1_001_000_014,
ctx: 'Connection 0',
msg: 'Connecting',
msg: 'Connecting Started',
attr: (actual: any) => {
expect(actual.url).to.match(/^mongodb:\/\/127.0.0.1:27091/);
expect(actual.csfle).to.equal(null);
expect(actual).to.have.property('connectionId', 0);
},
},
{
Expand Down Expand Up @@ -284,8 +285,9 @@ describe('Logging and Telemetry integration', function () {
c: 'COMPASS-DATA-SERVICE',
id: 1_001_000_015,
ctx: 'Connection 0',
msg: 'Connected',
msg: 'Connecting Succeeded',
attr: {
connectionId: 0,
isMongos: false,
isWritable: true,
},
Expand Down
113 changes: 113 additions & 0 deletions packages/data-service/src/data-service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { mochaTestServer } from '@mongodb-js/compass-test-server';
import type { SearchIndex } from './search-index-detail-helper';
import { range } from 'lodash';
import ConnectionString from 'mongodb-connection-string-url';
import type { DataServiceImplLogger, MongoLogId } from './logger';

const { expect } = chai;
chai.use(chaiAsPromised);
Expand Down Expand Up @@ -93,6 +94,118 @@ describe('DataService', function () {
.drop();
});

describe('#connect', function () {
const connectionStartedId = 1_001_000_014;
const connectionSucceededId = 1_001_000_015;
const connectionFailedId = 1_001_000_359;
const logs: {
info: [
component: string,
id: MongoLogId,
context: string,
message: string,
attr?: unknown
];
}[] = [];
const logCollector: DataServiceImplLogger = {
error: () => {},
info: (...args) => logs.push({ info: args }),
debug: () => {},
warn: () => {},
fatal: () => {},
};

let dataServiceLogTest;

beforeEach(function () {
logs.length = 0;
});

afterEach(async function () {
await dataServiceLogTest?.disconnect();
dataServiceLogTest = undefined;
});

// The log and connection ids in this test are used by CompassWeb to measure connect time metrics.
// The intention of these tests is not to restrict any changes to the logs or ids.
// If a change to the ids is necessary please inform the appropriate team and they can adjust before pulling in the change.
it('when connecting succeeds there is a start and success log with a connectionId', async function () {
dataServiceLogTest = new DataServiceImpl(
connectionOptions,
logCollector
);
await dataServiceLogTest.connect();

expect(logs.map(({ info: [, id] }) => id.__value)).to.include(
connectionStartedId
);
expect(logs.map(({ info: [, id] }) => id.__value)).to.include(
connectionSucceededId
);

const startedLog = logs.find(
({ info: [, id] }) => id.__value === connectionStartedId
);
const succeededLog = logs.find(
({ info: [, id] }) => id.__value === connectionSucceededId
);

const { info: [, , , , startedAttr] = [] } = startedLog ?? {};
expect(startedAttr).to.have.property(
'connectionId',
dataServiceLogTest._id
);
const { info: [, , , , succeededAttr] = [] } = succeededLog ?? {};
expect(succeededAttr).to.have.property(
'connectionId',
dataServiceLogTest._id
);
});

it('when connecting fails there is a start and failure log with a connectionId', async function () {
dataServiceLogTest = new DataServiceImpl(
{
connectionString:
'mongodb://iLoveJavascript?serverSelectionTimeoutMS=5',
lookup: () => {
throw new Error('test error');
},
},
logCollector
);

const result = await dataServiceLogTest
.connect()
.catch((error) => error);
expect(result).to.be.instanceOf(Error);

expect(logs.map(({ info: [, id] }) => id.__value)).to.include(
connectionStartedId
);
expect(logs.map(({ info: [, id] }) => id.__value)).to.include(
connectionFailedId
);

const startedLog = logs.find(
({ info: [, id] }) => id.__value === connectionStartedId
);
const failedLog = logs.find(
({ info: [, id] }) => id.__value === connectionFailedId
);

const { info: [, , , , startedAttr] = [] } = startedLog ?? {};
expect(startedAttr).to.have.property(
'connectionId',
dataServiceLogTest._id
);
const { info: [, , , , succeededAttr] = [] } = failedLog ?? {};
expect(succeededAttr).to.have.property(
'connectionId',
dataServiceLogTest._id
);
});
});

describe('#isConnected', function () {
let dataServiceIsConnected: DataService;

Expand Down
19 changes: 17 additions & 2 deletions packages/data-service/src/data-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1579,7 +1579,8 @@ class DataServiceImpl extends WithLogContext implements DataService {
debug('connecting...');
this._isConnecting = true;

this._logger.info(mongoLogId(1_001_000_014), 'Connecting', {
this._logger.info(mongoLogId(1_001_000_014), 'Connecting Started', {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am inspired here by the driver's commandStarted/Succeeded/Failed. But the message changes aren't necessary so happy to undo.

Copy link
Collaborator

@gribnoysup gribnoysup May 23, 2025

Choose a reason for hiding this comment

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

If the log id stays the same and conceptually it's the same event I think that's fine (but you will need to figure out all the tests that might be checking this 🙂)

connectionId: this._id,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IIUC this _id is per attempt not per cluster, but that's fine for the use case in mms. LMK if "connectionId" isn't clear, I think its just important to not conflate that with a clusterId of some kind

url: redactConnectionString(this._connectionOptions.connectionString),
csfle: this._csfleLogInformation(this._connectionOptions.fleOptions),
});
Expand All @@ -1599,11 +1600,16 @@ class DataServiceImpl extends WithLogContext implements DataService {
});

const attr = {
connectionId: this._id,
isWritable: this.isWritable(),
isMongos: this.isMongos(),
};

this._logger.info(mongoLogId(1_001_000_015), 'Connected', attr);
this._logger.info(
mongoLogId(1_001_000_015),
'Connecting Succeeded',
attr
);
debug('connected!', attr);

state.oidcPlugin.logger.on('mongodb-oidc-plugin:state-updated', () => {
Expand All @@ -1626,6 +1632,15 @@ class DataServiceImpl extends WithLogContext implements DataService {
this,
this._crudClient
);
} catch (error) {
this._logger.info(mongoLogId(1_001_000_359), 'Connecting Failed', {
connectionId: this._id,
error:
error && typeof error === 'object' && 'message' in error
? error?.message
: 'unknown error',
});
throw error;
} finally {
this._isConnecting = false;
}
Expand Down
Loading