Skip to content
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

Remove checks for unsupported node versions #3720

Closed
dyladan opened this issue Apr 7, 2023 · 3 comments
Closed

Remove checks for unsupported node versions #3720

dyladan opened this issue Apr 7, 2023 · 3 comments
Assignees
Labels
contribfest These small and isolated issues are suitable for Kubecon Contribfest never-stale sdk:logs Issues and PRs related to the Logs SDK sdk:metrics Issues and PRs related to the Metrics SDK sdk:traces Issues and PRs related to the Traces SDK triage:accepted This feature has been accepted

Comments

@dyladan
Copy link
Member

dyladan commented Apr 7, 2023

We'll - unfortunately - have to postpone removing the checks from the actual code until we release the next major version.

Originally posted by @pichlermarc in #3689 (comment)

We should apply this to all the SDK components where applicable (might only be tracing)

THIS IS A BREAKING CHANGE. PLEASE MAKE PR TO THE next BRANCH FOR INCLUSION IN THE 2.0 RELEASE

@dyladan dyladan added this to the OpenTelemetry SDK 2.0 milestone Apr 7, 2023
@dyladan dyladan added never-stale sdk:metrics Issues and PRs related to the Metrics SDK sdk:traces Issues and PRs related to the Traces SDK sdk:logs Issues and PRs related to the Logs SDK labels Apr 7, 2023
@dyladan
Copy link
Member Author

dyladan commented Apr 7, 2023

Saved patch from #3689 in case the branch is deleted:

diff --git a/experimental/packages/opentelemetry-instrumentation-http/package.json b/experimental/packages/opentelemetry-instrumentation-http/package.json
index 55c3a113b..1836568d4 100644
--- a/experimental/packages/opentelemetry-instrumentation-http/package.json
+++ b/experimental/packages/opentelemetry-instrumentation-http/package.json
@@ -53,7 +53,6 @@
     "@types/mocha": "10.0.0",
     "@types/node": "18.6.5",
     "@types/request-promise-native": "1.0.18",
-    "@types/semver": "7.3.9",
     "@types/sinon": "10.0.13",
     "@types/superagent": "4.1.13",
     "axios": "0.24.0",
@@ -75,8 +74,7 @@
   "dependencies": {
     "@opentelemetry/core": "1.11.0",
     "@opentelemetry/instrumentation": "0.37.0",
-    "@opentelemetry/semantic-conventions": "1.11.0",
-    "semver": "^7.3.5"
+    "@opentelemetry/semantic-conventions": "1.11.0"
   },
   "homepage": "https://github.com/open-telemetry/opentelemetry-js/tree/main/experimental/packages/opentelemetry-instrumentation-http",
   "sideEffects": false
diff --git a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts
index 1f6affef6..eed6f34f9 100644
--- a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts
+++ b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts
@@ -38,7 +38,6 @@ import {
 import type * as http from 'http';
 import type * as https from 'https';
 import { Socket } from 'net';
-import * as semver from 'semver';
 import * as url from 'url';
 import {
   Err,
@@ -580,18 +579,6 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
         options,
         extraOptions
       );
-      /**
-       * Node 8's https module directly call the http one so to avoid creating
-       * 2 span for the same request we need to check that the protocol is correct
-       * See: https://github.com/nodejs/node/blob/v8.17.0/lib/https.js#L245
-       */
-      if (
-        component === 'http' &&
-        semver.lt(process.version, '9.0.0') &&
-        optionsParsed.protocol === 'https:'
-      ) {
-        return original.apply(this, [optionsParsed, ...args]);
-      }
 
       if (
         utils.isIgnored(
diff --git a/experimental/packages/otlp-exporter-base/src/platform/node/util.ts b/experimental/packages/otlp-exporter-base/src/platform/node/util.ts
index fd40981e8..18e90f2a3 100644
--- a/experimental/packages/otlp-exporter-base/src/platform/node/util.ts
+++ b/experimental/packages/otlp-exporter-base/src/platform/node/util.ts
@@ -50,7 +50,6 @@ export function sendWithHttp<ExportItem, ServiceRequest>(
 ): void {
   const exporterTimeout = collector.timeoutMillis;
   const parsedUrl = new url.URL(collector.url);
-  const nodeVersion = Number(process.versions.node.split('.')[0]);
   let retryTimer: ReturnType<typeof setTimeout>;
   let req: http.ClientRequest;
   let reqIsDestroyed = false;
@@ -63,8 +62,7 @@ export function sendWithHttp<ExportItem, ServiceRequest>(
       const err = new OTLPExporterError('Request Timeout');
       onError(err);
     } else {
-      // req.abort() was deprecated since v14
-      nodeVersion >= 14 ? req.destroy() : req.abort();
+      req.destroy();
     }
   }, exporterTimeout);

@dyladan dyladan added triage triage:accepted This feature has been accepted and removed triage labels Sep 27, 2023
@dyladan dyladan self-assigned this Oct 25, 2023
@dyladan dyladan added the contribfest These small and isolated issues are suitable for Kubecon Contribfest label Nov 8, 2023
@dyladan
Copy link
Member Author

dyladan commented Nov 22, 2023

Also consider remove AsyncHooksContextManager because it will no longer be used.

@dyladan
Copy link
Member Author

dyladan commented Dec 13, 2023

Fixed in #4341

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribfest These small and isolated issues are suitable for Kubecon Contribfest never-stale sdk:logs Issues and PRs related to the Logs SDK sdk:metrics Issues and PRs related to the Metrics SDK sdk:traces Issues and PRs related to the Traces SDK triage:accepted This feature has been accepted
Projects
None yet
Development

No branches or pull requests

1 participant