feat(instrumentation-tls): wrap tls.connect API

[svrnm]

Which problem is this PR solving?

This PR is addressing the instrumentation request at #292

Short description of the changes

• Add plugin opentelemetry-instrumentation-tls
• Add example "network" that combines http, dns, tls & net packages:

Open Questions

• For whatever reason I needed to use prependOnceListener to get the instrumentation for the secureConnect event working. I didn't run into any issues with that, but maybe someone can double check?

• This package has a kind of dependency on the "net" plugin. Since both instrument the connect method of the socket I had to add a workaround to make both work at the same time. I am not super happy with that solution: I check in the isWrapped conditional for the name of the existing wrapper and if it is 'patchedConnect' I save it and apply it later in a "chained" fashion. Is there already an approach for dependencies?

          if (isWrapped(moduleExports.TLSSocket.prototype.connect)) {
            if (
              moduleExports.TLSSocket.prototype.connect.name ===
              'patchedConnect'
            ) {
              netPatchedConnect = moduleExports.TLSSocket.prototype.connect;
            }
            this._unwrap(moduleExports.TLSSocket.prototype, 'connect');
          }
...
return safeExecuteInTheMiddle(
          () =>
            netPatchedConnect
              ? netPatchedConnect.apply(this, args)
              : original.apply(this, args),
...

[codecov]

Codecov Report

Merging #447 (1133b5d) into main (1121ac9) will increase coverage by 0.05%.
The diff coverage is n/a.

❗ Current head 1133b5d differs from pull request most recent head 9e9c55d. Consider uploading reports for the commit 9e9c55d to get more accurate results

@@            Coverage Diff             @@
##             main     #447      +/-   ##
==========================================
+ Coverage   95.20%   95.25%   +0.05%     
==========================================
  Files         130      134       +4     
  Lines        8148     8385     +237     
  Branches      788      812      +24     
==========================================
+ Hits         7757     7987     +230     
- Misses        391      398       +7     

Impacted Files	Coverage Δ
node/opentelemetry-instrumentation-net/src/net.ts	98.07% <0.00%> (-0.58%)	⬇️
...ode/opentelemetry-instrumentation-net/src/types.ts	100.00% <0.00%> (ø)
...metry-instrumentation-document-load/src/version.ts	100.00% <0.00%> (ø)
...-instrumentation-document-load/src/documentLoad.ts	99.00% <0.00%> (ø)
...opentelemetry-instrumentation-net/test/tls.test.ts	93.24% <0.00%> (ø)
...entation-document-load/src/enums/AttributeNames.ts	100.00% <0.00%> (ø)
...de/opentelemetry-instrumentation-net/test/utils.ts	97.61% <0.00%> (+1.46%)	⬆️

[obecny]

first pass, why the test coverage went down so much ?

[obecny]

This package has a kind of dependency on the "net" plugin.

Maybe consider adding the tls under net plugin ?

[obecny]

Patching the same thing twice for different instrumentations with shimmer it is tricky as unpatching cannot be done then
properly in different order
plugin 1 patch -> plugin 2 patch -> plugin 2 unpatch -> plugin 1 unpatch / ok
plugin 1 patch -> plugin 2 patch -> plugin 1 unpatch // error

@open-telemetry/javascript-approvers replacing shimmer sooner or later is inevitable. I have mentioned this year ago, worth to mention it again, it will work fine with mpwrapper for example, allowing for such things without any problem

[svrnm]

Maybe consider adding the tls under net plugin ?
@obecny I'll give it a try. Checking if a Socket is an instance of TSLSocket and then hooking into it might be the better approach.

[svrnm]

@obecny I figured out a way to combine TLS into the .net package (like http instrumentation does both http and https)

I am not 100% sure how this works: the attributes for TLS (like cipher and certificate details) are not unique to Node.JS of course. If I understand it correctly, they might be a candidate for semantic-attributes, which are defined through the specification, right?

[vmarchaud]

I am not 100% sure how this works: the attributes for TLS (like cipher and certificate details) are not unique to Node.JS of course. If I understand it correctly, they might be a candidate for semantic-attributes, which are defined through the specification, right?

There indeed defined at the spec level (here for trace and here for resources) which we then use to code-gen our package (here), you should open an issue there to be able to "standardize" them for all SDKs. In the mean time, i think you can keep the one you used

[dyladan]

Tests are timing out after 360 minutes. i'm going to manually trigger them again but if they time out again you probably have something to look into.

[svrnm]

Tests are timing out after 360 minutes. i'm going to manually trigger them again but if they time out again you probably have something to look into.

Thanks. I saw this issue but couldn’t figure out yet why this happens. I’ll try to reproduce it locally and come up with a fix.

[dyladan]

Are any of these in the Semantic Conventions?

[svrnm]

Not that I am aware of. I checked the specs and didn't find them there

[dyladan]

can you add a comment that these are not "official"

we need guidance from the spec how to add attributes that are not yet specified. If the spec eventually adds one of these attributes and it isn't compatible with ours that will be a problem.

@open-telemetry/technical-committee any guidance how to handle this?

[svrnm]

Sorry, I was confused. I thought you talk about the socket events, but you mean the TLSAttributes? I actually opened an issue already: open-telemetry/opentelemetry-specification#1652

can you add a comment that these are not "official"

Yes, I can do that.

I guess they are not the only candidates for that, e.g. the DNS package puts the name that was looked up into net.peer.name, which is probably not correct and their might be a semantic convention for DNS.

Since this is kind of related to open-telemetry/opentelemetry-js#2123: Would it help if I scan js and js-contrib (semi)automatically for attributes that are either in the semantic-specs already or that might be candidates for it?

[dyladan]

that would be great

[svrnm]

@dyladan I found a bunch of them. There's four different cases:

1. Use of existing semantic conventions (like PG, see Fix semantic conventions for PG and PG Pool #467)
2. Use of potentially new semantic conventions, I mean things that are used outside of node.js as well (DNS, TLS, graphQL)
3. Use of custom attributes specific to the instrumented library (express, hapi, restify)
4. A corner case are browser specific attributes: while those are "unique" to javascript I would still argue that they should be covered by a semantic convention, since they are still universal

Since this is a discussion unrelated to this ticket, can you give me some guidance where to move this discussion? Open another issue with the specification repo?

[dyladan]

I would open an issue on this repo to track it at least. If you think some should be spec'd then you should open an issue on the spec repo too and link them.

[svrnm]

Sorry for triggering the tests multiple times, I assumed no issues with node:10 and node:8, hope it's fixed now...