feat(opentelemetry-instrumentation): replace `semver` package with internal semantic versioning check implementation #5305

serkan-ozal · 2025-01-08T17:33:27Z

Which problem is this PR solving?

I am one of the OTEL FAAS SIG members and working on reducing coldstart overhead of the OTEL Lambda Node.js layer.

During my analysis, I have noticed that semver package has some initialization overhead (~15 ms) and most of it is caused by the semver internal initialization here (You can see that there are many RegExp compiles there) .

So I have been looking for way to reduce it and I believe that getting rid of semver dependency and providing an internal and simpler semantic versioning check implementation makes more sense.

Short description of the changes

This PR removes semver package dependency and replaces its usages with internal semantic versioning implementation.

Some parts of the internal semver implenentation is borrowed from actual semver package code base.

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

experimental/packages/opentelemetry-instrumentation/test/common/semver.test.ts

Checklist:

Followed the style guidelines of this project
Unit tests have been added
Documentation has been updated

RaphaelManke · 2025-01-08T18:26:45Z

...rimental/packages/opentelemetry-instrumentation/test/common/fixtures/semver-range-exclude.js

+/*
+ * 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.
+ */


Is this the correct Licence notice? The original one from semver says you have to mention theirs as well.

The ISC License Copyright (c) Isaac Z. Schlueter and Contributors Permission to use, copy, modify, and/or distribute this software for any purpose with or without fee is hereby granted, provided that the above copyright notice and this permission notice appear in all copies. THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

Same for the other copied and modified files.

Hmm, I am not very familiar these license stuff. Shoıld I just append semver License header to the OTEL header? Is there anything I should do?

Unfortunately I don't know either. And a google search didn't give me an answer.
I just wanted to highlight that there is maybe something to do, to mitigate the risk of a license issue.

Per https://github.com/open-telemetry/community/blob/main/guides/contributor/processes.md#copyright-notices

If you make substantial changes to the third-party code, prepend the contributed third party file with OpenTelemetry's copyright notice.

Here is an example:
https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-aws-sdk/src/propwrap.ts

However, the same community repo link above also says:

Any contributed third-party code must originally be Apache 2.0-Licensed or must carry a permissive software license that is compatible when combining with Apache 2.0 License. At this moment, BSD and MIT are the only OSI-approved licenses known to be compatible.

Unfortunately this is neither BSD or MIT.
ISC is an OSI approved license (https://opensource.org/license/isc-license-txt) and https://en.wikipedia.org/wiki/ISC_license suggests it is "It is functionally equivalent to the simplified BSD and MIT licenses, but without language deemed unnecessary following the Berne Convention."

Options I see:

Ask the OTel TC and/or GC for advice here on whether the ISC license could reasonably be added to that list of licenses "known to be compatible". @mx-psi Do you have any experience here with this kind of license question?

Publish this separate simple semver-satisfies as a separate package to npm and add a dependency on it. I don't know if you'd be willing to do that.

@trentm Just pinged @mx-psi in the CNCF Slack about this. Let's see whether this is problematic and if so, how we can handle it.

Hey, thanks for the ping. I'll preface this by saying that I am just stating my personal opinion and I am not giving legal advice nor representing the GC here.

AIUI this is not contributed code, but rather code you are using as a dependency of some sort. For that case, you can look into the CNCF third party license guidelines. ISC is listed there: https://github.com/cncf/foundation/blob/main/allowed-third-party-license-policy.md#cncf-allowlist-license-policy as an "Approved [License] for Allowlist". If you are able to fulfill the rest of the criteria mentioned in that document (e.g. by "storing [the code] unmodified in a designated third-party folder") and checking that (3) is satisfied, then I think you should be good.

There's also some dependencies that are specifically approved (see here), I don't think this is one of them, but feel free to check it as well.

@trentm As pointed here, I have added license headers and info for the imported codes.

Could you please check that.

codecov · 2025-01-08T18:40:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.56%. Comparing base (ec638dc) to head (6070a9d).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5305   +/-   ##
=======================================
  Coverage   94.56%   94.56%           
=======================================
  Files         322      322           
  Lines        8132     8132           
  Branches     1715     1715           
=======================================
  Hits         7690     7690           
  Misses        442      442

trentm

I'm supportive of this.

There is the license/copyright Q to sort out.
I'd love to have some details on the semver.ts implementation to know if I should more closely review it.

Other comments are nits.

trentm · 2025-01-08T23:15:41Z

experimental/packages/opentelemetry-instrumentation-grpc/package.json

@@ -59,13 +59,11 @@
    "@protobuf-ts/runtime-rpc": "2.9.4",
    "@types/mocha": "10.0.10",
    "@types/node": "18.6.5",
-    "@types/semver": "7.5.8",


Ah, you were doing this too. I noticed that some other packages had now-unused deps on semver and opened #5306
I should have read your patch first. :)

Yep, there were some unused semver deps and some of them are only used by tests and can easily be replaced.

trentm · 2025-01-08T23:29:44Z

experimental/packages/opentelemetry-instrumentation/src/semver.ts

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+


As being discussed in a separate thread (on the test file), we will need to decide on the license/copyright.

Is this implementation significantly copied from node-semver?
That's relevant both for license/copyright and for helping review this. I'm not sure how carefully I should review this (it is a fair amount of code). Passing all the semver.satisfies tests is very nice.

@trentm
The following methods have been copied with very minor modifications:

replaceTilde

replaceCaret

replaceXRange

replaceHyphen

regexp definitions used by them

@trentm

I have also another custom semver implementation (without any code part is imported from actual semver package, but some parts of it is a little bit hacky) and all semver.satisfies tests are also passing with it too. But the problem is that there are many specific rules by semver package and some of them are not explicitly mentioned in the README and hidden in their code.

So, even though semver.satisfies all tests are passing, I am not very confident about how my other custom semver implementation is compatible with actual semver package if there are cases which are not tested.

trentm · 2025-01-08T23:31:11Z

experimental/packages/opentelemetry-instrumentation/src/semver.ts

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+


A comment near the top of the file giving the basic justification for this semver.ts (i.e. that it is about startup time vs. node-semver, limited to just satisfies(), etc.) would be helpful for future maintainers.

experimental/packages/opentelemetry-instrumentation/src/types.ts

trentm · 2025-01-08T23:37:39Z

experimental/packages/opentelemetry-instrumentation/test/common/semver.test.ts

+        }
+      );
+    });
+  });


Should these tests: https://github.com/npm/node-semver/blob/main/test/functions/satisfies.js#L17-L28
also be covered as well?

trentm · 2025-01-08T23:39:39Z

experimental/packages/opentelemetry-instrumentation/test/common/semver.test.ts

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+


A comment that this is meant to mirror https://github.com/npm/node-semver/blob/main/test/functions/satisfies.js would be helpful.

…ementation

Co-authored-by: Trent Mick <[email protected]>

serkan-ozal requested a review from a team as a code owner January 8, 2025 17:33

serkan-ozal force-pushed the feat/get-rid-of-semver branch from d683ca8 to 5a35b91 Compare January 8, 2025 17:56

RaphaelManke reviewed Jan 8, 2025

View reviewed changes

trentm mentioned this pull request Jan 8, 2025

chore(deps): remove unused 'semver' deps from some packages #5306

Merged

trentm requested changes Jan 8, 2025

View reviewed changes

serkan-ozal force-pushed the feat/get-rid-of-semver branch from c1bd858 to 08168bc Compare January 9, 2025 08:33

serkan-ozal changed the title ~~Replace semver package with internal semantic versioning check implementation~~ feat(opentelemetry-instrumentation): replace semver package with internal semantic versioning check implementation Jan 9, 2025

serkan-ozal force-pushed the feat/get-rid-of-semver branch from cae5bee to 7cb6dd7 Compare January 9, 2025 10:21

serkan-ozal mentioned this pull request Jan 9, 2025

[Licensing Question] Import semver package partially into code base open-telemetry/community#2504

Open

serkan-ozal force-pushed the feat/get-rid-of-semver branch 3 times, most recently from 512d92b to 098d5cd Compare January 10, 2025 16:39

serkan-ozal and others added 5 commits January 10, 2025 19:54

Replace semver package with internal semantic versioning check impl…

00b38ec

…ementation

Update experimental/packages/opentelemetry-instrumentation/src/types.ts

f46808f

Co-authored-by: Trent Mick <[email protected]>

Review comments

1b3b96d

Add changelog entry

ecabb39

Add license headers and info

678edf6

serkan-ozal force-pushed the feat/get-rid-of-semver branch from 098d5cd to 678edf6 Compare January 10, 2025 16:54

Merge branch 'main' into feat/get-rid-of-semver

6070a9d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(opentelemetry-instrumentation): replace `semver` package with internal semantic versioning check implementation #5305

feat(opentelemetry-instrumentation): replace `semver` package with internal semantic versioning check implementation #5305

serkan-ozal commented Jan 8, 2025 •

edited

Loading

RaphaelManke Jan 8, 2025

serkan-ozal Jan 8, 2025

RaphaelManke Jan 8, 2025

trentm Jan 8, 2025

serkan-ozal Jan 9, 2025

mx-psi Jan 9, 2025

serkan-ozal Jan 9, 2025

codecov bot commented Jan 8, 2025 •

edited

Loading

trentm left a comment

trentm Jan 8, 2025

serkan-ozal Jan 9, 2025 •

edited

Loading

trentm Jan 8, 2025

serkan-ozal Jan 9, 2025

serkan-ozal Jan 9, 2025

trentm Jan 8, 2025

serkan-ozal Jan 9, 2025

trentm Jan 8, 2025

serkan-ozal Jan 9, 2025

trentm Jan 8, 2025

serkan-ozal Jan 9, 2025

feat(opentelemetry-instrumentation): replace semver package with internal semantic versioning check implementation #5305

Are you sure you want to change the base?

feat(opentelemetry-instrumentation): replace semver package with internal semantic versioning check implementation #5305

Conversation

serkan-ozal commented Jan 8, 2025 • edited Loading

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 8, 2025 • edited Loading

Codecov Report

trentm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serkan-ozal Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(opentelemetry-instrumentation): replace `semver` package with internal semantic versioning check implementation #5305

feat(opentelemetry-instrumentation): replace `semver` package with internal semantic versioning check implementation #5305

serkan-ozal commented Jan 8, 2025 •

edited

Loading

codecov bot commented Jan 8, 2025 •

edited

Loading

serkan-ozal Jan 9, 2025 •

edited

Loading