fix: Work with optional dependencies #2
Conversation
RafalSumislawski
changed the title
|
Thanks so much for this fix! Would you mind adding a test for a mongo span being emitted in |
|
I would love to do that, but it's problematic. Unlike |
Oh I see, didn't realize that. In my local fork of this I had been spinning up redis/localstack in containers that my CI job could talk to, but I also didn't see a precedent for that in the OTel repos. I can't think of a better solution than this. What would you think of leaving a comment as something that would be nice to add in the future but we can merge as is? |
|
Sure, I'll leave a comment. BTW have you researched the CodeQL issue. I saw you commented about it in that PR. I think CodeQL has no supression mechanism, but maybe we can "fix" the issue. I mean apply some code change that would result in that no longer triggering. |
|
I have not, if you're up for a way to get around it that'd be awesome, but also fine with me if you want to merge as is and I can handle it in the destination branch |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Also add a TODO about mongodb test move code around move code around Import instead of require make prettier happy prettier test remove commented code
RafalSumislawski
force-pushed
the
work-with-optional-dependencies
branch
from
cc62cad to
715d18a
Compare
|
@drewcorlin1 I think this PR is ready I was able to make CodeQL understand that we have rate limiting by chaining the |
|
@RafalSumislawski is this working when you run it locally? I'm trying to add a test using mongodb-memory-server but still seeing nothing being emitted. /*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { MongoClient } from 'mongodb';
import { MongoMemoryServer } from 'mongodb-memory-server';
import { buildTestSchema } from './graphql/schema';
import { createClient } from 'redis';
import fastify from 'fastify';
import { graphql } from './graphql/adapter';
import pino from 'pino';
const redisClient = createClient({ url: process.env.REDIS_URL });
export const server = fastify({
logger: pino({}, pino.destination(1)),
disableRequestLogging: true,
}).register(require('@fastify/rate-limit'), {
// Use @fastify/rate-limit to avoid CodeQL "Missing rate limiting" error in CI
global: true,
max: 100,
timeWindow: '1 minute',
});
server.get('/test', async req => {
req.log.info({ hi: 'there' }, 'Log message from handler');
await redisClient.get('key').catch(() => void 0);
try {
// TODO: fix
// @ts-ignore
const mongoDbUri = req.query?.mongoDbUri;
if (!mongoDbUri) throw new Error('Missing mongoDbUri query param');
const mongoClient = new MongoClient(mongoDbUri);
const db = mongoClient.db('sample-database');
const col = db.collection('sample-collection');
const result = await col.findOne({ hello: 'test' });
await mongoClient.close();
} catch (e) {
req.log.info(e, 'Error connecting to MongoDB');
}
return { hi: 'there' };
});
const schema = buildTestSchema();
const sourceList = `
query {
books {
name
}
}
`;
server.get('/graphql', async req => {
await graphql({ schema, source: sourceList });
return { success: true };
});
server
.listen({ port: 8080 })
.then(async () => {
const mongod = await MongoMemoryServer.create();
const mongoDbUri = mongod.getUri();
try {
await server
.inject()
.get('/test')
.query({ mongoDbUri })
.end()
.catch(err => {
throw err;
});
await server
.inject()
.get('/graphql')
.end()
.catch(err => {
throw err;
});
} finally {
await mongod.stop().catch(() => void 0);
return server.close();
}
})
.catch(err => {
throw err;
}); |
|
As I mentioned in the PR's description, this PR just makes it not crash in bundle-time. For the instrumentation to actually work, you also need #3 |
|
Missed that it wasn't rebased, thanks for this! pushed a test for the mongoDB test to the |
Some libraries have option dependencies which they
requirein atry. For examplemongodbrequiresmongodb-client-encryptionthis way here: https://github.com/mongodb/node-mongodb-native/blob/main/src/deps.ts#L277With the current implementation of
esbuild-node-pluginusingmongodbleads to a bundling error:Catching the error thrown in
extractPackageAndModulePathand letting the blunder continue resolves the problem.There is an issue about providing esbuild plugins with better ways to handle this evanw/esbuild#1127, but for now I don't see better alternatives.
Side note: this PR will make code using
mongodbbundle successfully and work in runtime, but it doesn't make themongodbinstrumentation work. I'll submit a fix for in the next PR.