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: bhaskarbanerjee / name: Bhaskar Banerjee (67ad4da, f00dc3d, a76c5b6)

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. 🙂

pichlermarc · 2024-11-19T10:27:20Z

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

+  if (enabledExporters.includes('none')) {
+    diag.info(
+      `OTEL_METRICS_EXPORTER contains "none". Metric provider will not be initialized.`
+    );


could we make this return here to reduce nesting below? 🙂

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

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

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

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

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

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

Are you sure you want to change the base?

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

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

bhaskarbanerjee commented Nov 15, 2024 •

edited by dyladan

Loading

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

edited

Loading