feat(opentelemetry-sdk-node): automatically configure metrics exporter based on environment variables #5168

bhaskarbanerjee · 2024-11-15T21:31:18Z

Which problem is this PR solving?

Provides ability to auto instrument metrics based on environment variables

Fixes #4551

Short description of the changes

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

Test A

Checklist:

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

linux-foundation-easycla · 2024-11-15T21:31:22Z

The committers listed above are authorized under a signed CLA.

✅ login: pichlermarc / name: Marc Pichler (d001055, 6f848e1)
✅ login: bhaskarbanerjee / name: Bhaskar Banerjee (0a47943, 4f05f24, f00dc3d, a76c5b6, 89a858f, 52f8a02, 67ad4da)

dyladan · 2024-11-15T23:09:58Z

experimental/packages/opentelemetry-sdk-node/src/sdk.ts

@@ -334,6 +350,99 @@ export class NodeSDK {
    );
  }

+  public configureMetricProviderFromEnv(): MetricReader[] {


This doesn't need to be a method on the class because it isn't accessing this. Making it a function improves minification and tree-shaking. It looks like the function wouldn't need to be exported too, keeping the external API surface as small as possible. It should at least not be public.

dyladan · 2024-11-15T23:11:22Z

experimental/packages/opentelemetry-sdk-node/src/sdk.ts

+  /**
+   * @Returns param value, if set else returns the default value
+   */
+  private getValueInMillis(envName: string, defaultValue: number): number {


This could just be a function because it doesn't access this

dyladan · 2024-11-15T23:13:56Z

experimental/packages/opentelemetry-sdk-node/tsconfig.json

-    {
-      "path": "../api-logs"
-    },


Did you mean to remove this?

No, that was a mistake

experimental/packages/opentelemetry-sdk-node/README.md

JacksonWeber · 2024-11-15T23:55:14Z

experimental/packages/opentelemetry-sdk-node/src/sdk.ts

+    // ONLY if OTEL_METRICS_EXPORTER is set
+    if (enabledExporters.length === 0) {
+      diag.info('OTEL_METRICS_EXPORTER is empty. Using default otlp exporter.');
+      // enabledExporters.push('otlp');


Clean up stray commented line of code.

experimental/packages/opentelemetry-sdk-node/src/sdk.ts

pichlermarc

thanks for working on this 🙌

pichlermarc · 2024-11-18T09:11:17Z

experimental/packages/opentelemetry-sdk-node/src/sdk.ts

+    // ONLY if OTEL_METRICS_EXPORTER is set
+    if (enabledExporters.length === 0) {


Hmm, this will always log though even if OTEL_METRICS_EXPORTER is undefined. I think it'd probably the best to check if process.env.OTEL_METRIC_EXPORTER is undefined and returning an empty array right at the beginning of the function to avoid that any log spurious log messages pop up that are not applicable to what the user actually configured.

experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts

pichlermarc · 2024-11-18T09:21:24Z

Please also sign the CLA, otherwise we'll not be able to proceed with this PR. 🙂

bhaskarbanerjee · 2024-11-18T22:58:14Z

@dyladan @pichlermarc @JacksonWeber Thanks for you review, I have updated the PR, seeking feedback.

pichlermarc

Thanks for addressing the comments 🙌

Thanks for signing the CLA. 👍 Looks like there's still one commit that does not have the CLA signed (is it using different e-mail, perhaps 🤔) - having all commits authorized by the CLA is a required check, so we'll not be able to merge this PR without that. Would you mind having another look? 🙂

experimental/packages/opentelemetry-sdk-node/src/sdk.ts

pichlermarc · 2024-11-19T10:13:18Z

experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts

+        sharedState.metricCollectors[0]._metricReader._exporter instanceof
+          ConsoleMetricExporter
+      );
+      assert(


please use assert.strictEqual() wherever possible in the tests that were added. 🙂

experimental/packages/opentelemetry-sdk-node/src/sdk.ts

bhaskarbanerjee · 2024-11-19T17:17:42Z

@pichlermarc
I am unsure how the commit 73c2071 made it from user bhskrbnrj

Those changes are mine but that is not my user id. Let me see if I can remove that commit from my PR.

bhaskarbanerjee · 2024-11-19T17:25:00Z

That PR is not from my fork (might be I was fiddling with the PR from UI). The use name seems automated (i.e. stripped off all vowels from my name). Are you OK if I raise a new PR (by including all of these PR comments in there?
I will keep this PR open until then. and once the other PR is merged, I will close this out.

Please advice @pichlermarc

bhaskarbanerjee · 2024-11-20T03:37:16Z

I had to rollback all commits in this PR because the first commit was bad (not from a legit fork). I shall soon update this PR with the latest code. @pichlermarc

bhaskarbanerjee · 2024-11-20T03:37:31Z

I had to rollback all commits in this PR because the first commit was bad (not from a legit fork). I shall soon update this PR with the latest code. @pichlermarc

bhaskarbanerjee · 2024-11-20T21:23:55Z

@pichlermarc I have cleaned up the bad PR and addressed all review comments. Seeking your review on this commit

pichlermarc

Thanks for addressing the comments - almost there.

We also need to add the now used exporters as dependencies to @opentelemetry/sdk-node's package.json. Please be sure to follow up with a npm install --package-lock-only after you do that and ensure that the changes made to package-lock.json are added to the commit. 🙂

experimental/packages/opentelemetry-sdk-node/README.md

pichlermarc · 2024-11-22T13:11:20Z

experimental/packages/opentelemetry-sdk-node/src/sdk.ts

+      `OTEL_METRICS_EXPORTER contains "none". Metric provider will not be initialized.`
+    );
+    return metricReaders;
+  } else {


we return in the if block above so we can reduce nesting here by dropping the else.

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

bhaskarbanerjee · 2024-11-22T14:42:59Z

Thanks for addressing the comments - almost there.

We also need to add the now used exporters as dependencies to @opentelemetry/sdk-node's package.json. Please be sure to follow up with a npm install --package-lock-only after you do that and ensure that the changes made to package-lock.json are added to the commit. 🙂

I ran npm install --package-lock-only but it didn't yield any change in package-lock.json.
I see that the newly added exporters have already been part of the package-lock.json

codecov · 2024-11-22T15:59:23Z

Codecov Report

Attention: Patch coverage is 95.83333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.67%. Comparing base (e4d9c21) to head (d001055).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...imental/packages/opentelemetry-sdk-node/src/sdk.ts	95.83%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5168   +/-   ##
=======================================
  Coverage   94.67%   94.67%           
=======================================
  Files         315      315           
  Lines        8012     8057   +45     
  Branches     1617     1632   +15     
=======================================
+ Hits         7585     7628   +43     
- Misses        427      429    +2

Files with missing lines	Coverage Δ
...imental/packages/opentelemetry-sdk-node/src/sdk.ts	`96.96% <95.83%> (-0.42%)`	⬇️

pichlermarc

Thanks, please also add an entry in experimental/CHANGELOG.md then this should be good to merge 🙂
Edit: I just checked locally and npm install --package-lock-only does in fact change package-lock.json with the current state of the PR. Would you mind having a look again? 🤔

bhaskarbanerjee · 2024-11-22T18:01:37Z

@pichlermarc This is the output of npm install --package-lock-only on my system

abcabc@mymac opentelemetry-js % nvm use 18                     
Now using node v18.20.2 (npm v10.5.0)
abcabc@mymac opentelemetry-js % npm install --package-lock-only
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'glob@11.0.0',
npm WARN EBADENGINE   required: { node: '20 || >=22' },
npm WARN EBADENGINE   current: { node: 'v18.20.2', npm: '10.5.0' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'minimatch@10.0.1',
npm WARN EBADENGINE   required: { node: '20 || >=22' },
npm WARN EBADENGINE   current: { node: 'v18.20.2', npm: '10.5.0' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'jackspeak@4.0.2',
npm WARN EBADENGINE   required: { node: '20 || >=22' },
npm WARN EBADENGINE   current: { node: 'v18.20.2', npm: '10.5.0' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'path-scurry@2.0.0',
npm WARN EBADENGINE   required: { node: '20 || >=22' },
npm WARN EBADENGINE   current: { node: 'v18.20.2', npm: '10.5.0' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'lru-cache@11.0.2',
npm WARN EBADENGINE   required: { node: '20 || >=22' },
npm WARN EBADENGINE   current: { node: 'v18.20.2', npm: '10.5.0' }
npm WARN EBADENGINE }
(node:69826) Warning: Setting the NODE_TLS_REJECT_UNAUTHORIZED environment variable to '0' makes TLS connections and HTTPS requests insecure by disabling certificate verification.
(Use `node --trace-warnings ...` to show where the warning was created)

> opentelemetry@0.1.0 postinstall
> npm run update-ts-configs


> opentelemetry@0.1.0 update-ts-configs
> node scripts/update-ts-configs.js


up to date, audited 2355 packages in 6s

224 packages are looking for funding
  run `npm fund` for details

66 vulnerabilities (5 low, 8 moderate, 18 high, 35 critical)

To address issues that do not require attention, run:
  npm audit fix

To address all issues possible (including breaking changes), run:
  npm audit fix --force

Some issues need review, and may require choosing
a different dependency.

Run `npm audit` for details.
abcabc@mymac opentelemetry-js % git diff                       
diff --git a/experimental/packages/opentelemetry-sdk-node/tsconfig.json b/experimental/packages/opentelemetry-sdk-node/tsconfig.json
index 7d18aac48..07232313f 100644
--- a/experimental/packages/opentelemetry-sdk-node/tsconfig.json
+++ b/experimental/packages/opentelemetry-sdk-node/tsconfig.json
@@ -60,18 +60,6 @@
     {
       "path": "../exporter-trace-otlp-proto"
     },
-    {
-      "path": "../opentelemetry-exporter-metrics-otlp-grpc"
-    },
-    {
-      "path": "../opentelemetry-exporter-metrics-otlp-http"
-    },
-    {
-      "path": "../opentelemetry-exporter-metrics-otlp-proto"
-    },
-    {
-      "path": "../opentelemetry-exporter-prometheus"
-    },
     {
       "path": "../opentelemetry-instrumentation"
     },

bhaskarbanerjee requested a review from a team as a code owner November 15, 2024 21:31

dyladan reviewed Nov 15, 2024

View reviewed changes

JacksonWeber reviewed Nov 15, 2024

View reviewed changes

experimental/packages/opentelemetry-sdk-node/README.md Outdated Show resolved Hide resolved

JacksonWeber reviewed Nov 15, 2024

View reviewed changes

pichlermarc reviewed Nov 18, 2024

View reviewed changes

experimental/packages/opentelemetry-sdk-node/src/sdk.ts Outdated Show resolved Hide resolved

experimental/packages/opentelemetry-sdk-node/src/sdk.ts Outdated Show resolved Hide resolved

pichlermarc reviewed Nov 18, 2024

View reviewed changes

pichlermarc reviewed Nov 19, 2024

View reviewed changes

bhaskarbanerjee closed this Nov 19, 2024

bhaskarbanerjee force-pushed the feature/auto_instrumentation_metrics branch from 41f0adf to 0ee9a27 Compare November 19, 2024 23:30

remove bad commit and address PR comments

67ad4da

bhaskarbanerjee reopened this Nov 20, 2024

Merge branch 'main' into feature/auto_instrumentation_metrics

f00dc3d

Merge branch 'main' into feature/auto_instrumentation_metrics

a76c5b6

pichlermarc requested changes Nov 22, 2024

View reviewed changes

bhaskarbanerjee and others added 3 commits November 22, 2024 08:30

Updated env vars descriptions

52f8a02

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

Drop unnecessary else check

89a858f

Adding new exporters as dependencies

0a47943

pichlermarc reviewed Nov 22, 2024

View reviewed changes

Add feature description to CHANGELOG.md

4f05f24

pichlermarc added 2 commits December 12, 2024 17:00

Merge branch 'main' into feature/auto_instrumentation_metrics

6f848e1

chore: sync package-lock.json

d001055

pichlermarc approved these changes Dec 12, 2024

View reviewed changes

pichlermarc added this pull request to the merge queue Dec 13, 2024

Merged via the queue into open-telemetry:main with commit 6d31a18 Dec 13, 2024
21 checks passed

opentelemetrybot mentioned this pull request Dec 16, 2024

chore: prepare next release #5274

Merged

pichlermarc mentioned this pull request Jan 7, 2025

update nodejs autoinstrumetation, fixes #2626 open-telemetry/opentelemetry-operator#3570

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(opentelemetry-sdk-node): automatically configure metrics exporter based on environment variables #5168

feat(opentelemetry-sdk-node): automatically configure metrics exporter based on environment variables #5168

bhaskarbanerjee commented Nov 15, 2024 •

edited by dyladan

Loading

linux-foundation-easycla bot commented Nov 15, 2024 •

edited

Loading

dyladan Nov 15, 2024

bhaskarbanerjee Nov 18, 2024

bhaskarbanerjee Nov 18, 2024

dyladan Nov 15, 2024

bhaskarbanerjee Nov 18, 2024

dyladan Nov 15, 2024

bhaskarbanerjee Nov 18, 2024

bhaskarbanerjee Nov 18, 2024

JacksonWeber Nov 15, 2024

bhaskarbanerjee Nov 18, 2024

pichlermarc left a comment

pichlermarc Nov 18, 2024

bhaskarbanerjee Nov 18, 2024

pichlermarc commented Nov 18, 2024

bhaskarbanerjee commented Nov 18, 2024

pichlermarc left a comment

pichlermarc Nov 19, 2024

bhaskarbanerjee Nov 19, 2024

bhaskarbanerjee commented Nov 19, 2024

bhaskarbanerjee commented Nov 19, 2024

bhaskarbanerjee commented Nov 20, 2024

bhaskarbanerjee commented Nov 20, 2024

bhaskarbanerjee commented Nov 20, 2024

pichlermarc left a comment

pichlermarc Nov 22, 2024

bhaskarbanerjee Nov 22, 2024

bhaskarbanerjee commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

pichlermarc left a comment •

edited

Loading

bhaskarbanerjee commented Nov 22, 2024

		// ONLY if OTEL_METRICS_EXPORTER is set
		if (enabledExporters.length === 0) {

feat(opentelemetry-sdk-node): automatically configure metrics exporter based on environment variables #5168

feat(opentelemetry-sdk-node): automatically configure metrics exporter based on environment variables #5168

Conversation

bhaskarbanerjee commented Nov 15, 2024 • edited by dyladan Loading

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

linux-foundation-easycla bot commented Nov 15, 2024 • 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

pichlermarc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pichlermarc commented Nov 18, 2024

bhaskarbanerjee commented Nov 18, 2024

pichlermarc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhaskarbanerjee commented Nov 19, 2024

bhaskarbanerjee commented Nov 19, 2024

bhaskarbanerjee commented Nov 20, 2024

bhaskarbanerjee commented Nov 20, 2024

bhaskarbanerjee commented Nov 20, 2024

pichlermarc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhaskarbanerjee commented Nov 22, 2024 • edited Loading

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

pichlermarc left a comment • edited Loading

Choose a reason for hiding this comment

bhaskarbanerjee commented Nov 22, 2024

bhaskarbanerjee commented Nov 15, 2024 •

edited by dyladan

Loading

linux-foundation-easycla bot commented Nov 15, 2024 •

edited

Loading

bhaskarbanerjee commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

pichlermarc left a comment •

edited

Loading