-
Notifications
You must be signed in to change notification settings - Fork 821
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add mongodb plugin #205
feat: add mongodb plugin #205
Conversation
7ba66bc
to
072fc10
Compare
Codecov Report
@@ Coverage Diff @@
## master #205 +/- ##
==========================================
+ Coverage 92.33% 92.44% +0.11%
==========================================
Files 146 145 -1
Lines 7047 7111 +64
Branches 621 621
==========================================
+ Hits 6507 6574 +67
+ Misses 540 537 -3
|
8b746cb
to
9bef9cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added initial comments, what are the version are the supported versions? I think we should target 1-3. Also, we should have unit tests for all supported versions. WDYT?
const error = args[0]; | ||
if (error instanceof Error) { | ||
span.setStatus({ | ||
code: CanonicalCode.UNKNOWN, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why CanonicalCode.UNKNOWN
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it hard to set the correct code depending on the mongodb error, and simpler to just set it as unknown. I don't think it would bring a lot of value to parse the error from mongodb to set the correct error code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR look good. Added comments.
|
||
/** MongoDB instrumentation plugin for OpenTelemetry */ | ||
export class MongoDBPlugin extends BasePlugin<MongoDB> { | ||
private readonly SERVER_FNS = ['insert', 'update', 'remove']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we should prefix private properties with "_"
9bef9cb
to
7bc2b08
Compare
Note that the plugin is currently not working, i will make the pr ready for review when it will be |
Some update on this: I have started to port opencensus code without success for now, i'm still checking if it should patch |
7bc2b08
to
a310cd8
Compare
Initial implementation is working, feel free to review it althrough i didn't finish yet, as @mayurkale22 said we need to add some tags on each span. The instrumentation is actually done against |
a310cd8
to
a687d05
Compare
private readonly _SERVER_METHODS = ['insert', 'update', 'remove', 'command']; | ||
private readonly _CURSOR_METHODS = ['_next', 'next']; | ||
|
||
protected readonly _supportedVersions = ['>=2 <3']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should use supportedVersions
from BasePlugin
and add tests in order to be sure that plugin is not loaded for other versions (if possible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to use supportedVersions
but i don't think we need to test that it behave correctly in the plugin, it would argue that it need to be done only where the behavior is implemented (ie. in PluginLoader)
a687d05
to
57d140b
Compare
First round of review has been taken into account ! Here are the attributes i add to each span:
|
30c2366
to
4952971
Compare
a44cbe0
to
303a123
Compare
I can't reproduce the linting issue in local, for me everything is fine. |
@vmarchaud after doing a |
303a123
to
febee90
Compare
@mayurkale22 Rebased, should be good to merge ! |
@vmarchaud do I understand correctly that this only instruments Looks like it was removed in mongodb/node-mongodb-native@16f9a1c#diff-b9cfc7f2cdf78a7f4b91a753d10865a2L20 |
@dyladan Yeah indeed, i didn't knew when i started the plugin that it was dropped from |
a6280c7
to
292bff1
Compare
I've rebased & addressed comments from @dyladan @markwolff, please review again ! thanks |
292bff1
to
0aa332d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just 2 minor questions
packages/opentelemetry-plugin-mongodb-core/test/mongodb.test.ts
Outdated
Show resolved
Hide resolved
@@ -5,7 +5,6 @@ | |||
"outDir": "build" | |||
}, | |||
"include": [ | |||
"src/**/*.ts", | |||
"test/**/*.ts" | |||
"src/**/*.ts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all tsconfig.ts
has "test" too, what's the reason of removing it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a typescript issue when compiling:
Argument of type 'SimpleSpanProcessor' is not assignable to parameter of type 'SpanProcessor'.
Types of property 'onStart' are incompatible.
Type '(span: import("/home/vmarchaud/opentelemetry/opentelemetry-js/packages/opentelemetry-plugin-mongodb-core/node_modules/@opentelemetry/tracing/build/src/Span").Span) => void' is not assignable to type '(span: import("/home/vmarchaud/opentelemetry/opentelemetry-js/packages/opentelemetry-types/build/src/trace/span").Span) => void'.
Types of parameters 'span' and 'span' are incompatible.
Type 'Span' is missing the following properties from type 'Span': _tracer, spanContext, kind, attributes, and 17 more.ts(2345)
Which makes no sense since i had the same setup as other plugins then i found out that compiling the test aren't useful since we run them using ts-mocha
which was working, i believe we could remove that from all other packages too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging PR now, please open an issue to discuss this separately.
0aa332d
to
4def83e
Compare
Plugin ported from OC: https://github.com/census-instrumentation/opencensus-node/tree/master/packages/opencensus-instrumentation-mongodb