refactor: migrate mongodb to instrumentation #250

[vmarchaud]

I migrated the plugin to instrumentation class, however i also updated the patch to hook to different files (which are more up to date with latest version and more precise).
It support mongodb starting 3.3 which was released in the summer of 2019. I though adding back support for < 3.3 but i think it make more sense in a seperate PR + see if supporting those old versions is actually needed.

[codecov]

Codecov Report

Merging #354 (682f44d) into main (19c5496) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #354   +/-   ##
=======================================
  Coverage   93.88%   93.88%           
=======================================
  Files          12       12           
  Lines         409      409           
  Branches       44       44           
=======================================
  Hits          384      384           
  Misses         25       25           

[vmarchaud]

About 6d7367b (#354): For some obscure reason, tests were failing if supportedVersions was a property, i tested with typescript 4.2.2 with the same result. I converted it to a getter which works for some reason.

[dyladan]

About 6d7367b (#354): For some obscure reason, tests were failing if supportedVersions was a property, i tested with typescript 4.2.2 with the same result. I converted it to a getter which works for some reason.

Can you paste the output of the broken test?

[vmarchaud]

Can you paste the output of the broken test?

You can see the tests output here: https://github.com/open-telemetry/opentelemetry-js-contrib/runs/1977512692?check_suite_focus=true

[Flarna]

This doesn't work on windows because of the /. Using \\ works (but most likely breaks non windows).

[dyladan]

Have you tested this on windows? I think this uses the node module resolver which I thought normalized paths.

[vmarchaud]

Even though i think its handled by node, if thats an actual bug we should fix this directly in the instrumentation base package because the mongodb plugin isn't the only one to use internals require

[Flarna]

Yes, tested on windows.

[vmarchaud]

@Flarna Could you open an issue on the main repo with the details ? we should fix this before RC of SDK for sure

[Flarna]

open-telemetry/opentelemetry-js#1990

[Flarna]

What about using path.normalize('mongodb/lib/core/wireprotocol/index.js'), for now? The turnaround time for a fix in core until it arrives in contrib is not that short.

[vmarchaud]

I'm fine doing that in another PR so we can do one PR for all contrib, WDYT ?

[obecny]

I think we should make a fix in core repo when InstrumentationNodeModuleFile is being created use path.normalize as @Flarna mentioned

[dyladan]

Can you document in the readme and in the package.json that this package is mac/linux only for now until the instrumentation package is updated?

[vmarchaud]

Can you document in the readme and in the package.json that this package is mac/linux only for now until the instrumentation package is updated?

I think it would be better to make a dedicated PR for that, the graphql and grpc instrumentation use internal require for sure and i'm not sure about others. WDYT ?

[Flarna]

I think it would be better to make a dedicated PR for that, the graphql and grpc instrumentation use internal require for sure and i'm not sure about others. WDYT ?

I can confirm that grpc instrumenation tests fail on windows because of the slashes issue.

[obecny]

generally looks fine, I have just one concern with dropping support for older version we did support with plugin

[vmarchaud]

Please don't merge the PR as i need to bump api dependency to 0.18