feat(instrumentation-tls): wrap tls.connect API by svrnm · Pull Request #447 · open-telemetry/opentelemetry-js-contrib

svrnm · 2021-04-23T07:49:18Z

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),
...

…lemetry-js-contrib into tls-library-instrumentation

codecov · 2021-04-23T13:40:33Z

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 · 2021-04-23T16:56:17Z

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

Maybe consider adding the tls under net plugin ?

obecny · 2021-04-23T17:03:51Z

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 · 2021-04-26T10:20:57Z

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.

…y-js-contrib into tls-library-instrumentation

svrnm · 2021-04-26T18:19:31Z

@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 · 2021-04-26T18:44:14Z

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 · 2021-04-29T20:51:44Z

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 · 2021-04-30T06:43:34Z

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.

…y-js-contrib into tls-library-instrumentation

…lemetry-js-contrib into tls-library-instrumentation

dyladan · 2021-04-30T12:22:52Z

+  SECURE_CONNECT = 'secureConnect',
+}
+
+export enum TLSAttributes {


Are any of these in the Semantic Conventions?

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

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?

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?

that would be great

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

Use of existing semantic conventions (like PG, see Fix semantic conventions for PG and PG Pool #467)

Use of potentially new semantic conventions, I mean things that are used outside of node.js as well (DNS, TLS, graphQL)

Use of custom attributes specific to the instrumented library (express, hapi, restify)

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?

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.

…l nodejs versions

svrnm · 2021-04-30T14:06:51Z

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

…icial

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

1ff9f9d

svrnm requested a review from a team April 23, 2021 07:49

svrnm added 3 commits April 23, 2021 09:49

Merge branch 'main' into tls-library-instrumentation

54315fa

style: remove unused import for connect.test.ts in tls plugin

326d0a8

Merge branch 'tls-library-instrumentation' of github.com:svrnm/opente…

dddc0fc

…lemetry-js-contrib into tls-library-instrumentation

obecny reviewed Apr 23, 2021

View reviewed changes

svrnm added 3 commits April 26, 2021 16:23

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

baeb05d

…y-js-contrib into tls-library-instrumentation

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

c5d9f44

…y-js-contrib into tls-library-instrumentation

feat(instrumentation-tls): move tls instrumentation into net package

81bd589

vmarchaud reviewed Apr 26, 2021

View reviewed changes

Comment thread plugins/node/opentelemetry-instrumentation-net/src/net.ts Outdated

feat(instrumentation-tls): remove comment in net.ts

5588ff0

svrnm mentioned this pull request Apr 27, 2021

Add semantic convention for describing SSL/TLS connections open-telemetry/opentelemetry-specification#1652

Closed

Merge branch 'main' into tls-library-instrumentation

2a5f840

vmarchaud approved these changes Apr 28, 2021

View reviewed changes

svrnm added 3 commits April 30, 2021 10:57

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

67b52d4

…y-js-contrib into tls-library-instrumentation

feat(instrumentation-tls): fix stalled tests

4d9fdad

Merge branch 'tls-library-instrumentation' of github.com:svrnm/opente…

01c13d4

…lemetry-js-contrib into tls-library-instrumentation

dyladan reviewed Apr 30, 2021

View reviewed changes

feat(instrumentation-tls): restructure tls tests and fix tests for al…

198dbee

…l nodejs versions

svrnm added 2 commits April 30, 2021 16:10

feat(instrumentation-tls): fix style

d6a3f50

feat(instrumentation-tls): add comment that TLSAttributes are not off…

6b40e75

…icial

svrnm mentioned this pull request Apr 30, 2021

improv: use semantic-conventions for resource attributes open-telemetry/opentelemetry-js#2123

Closed

Merge branch 'main' into tls-library-instrumentation

1353d56

svrnm mentioned this pull request May 5, 2021

Standardise implementation of non-official attributes open-telemetry/opentelemetry-js#2184

Closed

svrnm added 2 commits May 5, 2021 19:12

Merge branch 'main' into tls-library-instrumentation

08ac436

Merge branch 'main' into tls-library-instrumentation

62def9f

dyladan approved these changes May 5, 2021

View reviewed changes

Merge branch 'main' into tls-library-instrumentation

9e9c55d

vmarchaud merged commit 22fab71 into open-telemetry:main May 7, 2021

Conversation

svrnm commented Apr 23, 2021

Which problem is this PR solving?

Short description of the changes

Open Questions

Uh oh!

codecov Bot commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

obecny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

obecny commented Apr 23, 2021

Uh oh!

obecny commented Apr 23, 2021

Uh oh!

svrnm commented Apr 26, 2021

Uh oh!

svrnm commented Apr 26, 2021

Uh oh!

Uh oh!

vmarchaud commented Apr 26, 2021

Uh oh!

dyladan commented Apr 29, 2021

Uh oh!

svrnm commented Apr 30, 2021

Uh oh!

dyladan Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

svrnm Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

dyladan Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

svrnm Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

dyladan Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

svrnm May 4, 2021

Choose a reason for hiding this comment

Uh oh!

dyladan May 4, 2021

Choose a reason for hiding this comment

Uh oh!

svrnm commented Apr 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Apr 23, 2021 •

edited

Loading