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

[sdk-node] automatically configure metrics exporter based on enviornment variables #4551

Open
4 tasks
pichlermarc opened this issue Mar 18, 2024 · 29 comments
Open
4 tasks
Assignees
Labels
needs:code-contribution This feature/bug is ready to implement pkg:sdk-node triage:accepted This feature has been accepted type:feature A feature with no sub-issues to address

Comments

@pichlermarc
Copy link
Member

pichlermarc commented Mar 18, 2024

Currently, we don't auto-configure a MetricReader/exporter combination when using the @opentelemetry/sdk-node package.
Goal of this issue is to implement exporter selection for metrics based on this specification:

For this issue to be considered done we need to implement the following behavior:

If no metric reader is configured by the user:

@Prashansa-K
Copy link
Contributor

@pichlermarc I would like to work on this. I have used this package in the past.

@pichlermarc pichlermarc removed the up-for-grabs Good for taking. Extra help will be provided by maintainers label May 6, 2024
@pichlermarc
Copy link
Member Author

@Prashansa-K thanks for picking this up. It's yours 🙂

@Prashansa-K
Copy link
Contributor

Prashansa-K commented May 7, 2024

@pichlermarc
I have some doubts here.

  1. In first action item from the description mentions "use OTEL_METRICS_EXPORTER environment variable to determine an exporter and add it to the MeterProvider that's created by NodeSDK",
    However, as I see the code, addMetricReader() method is deprecated and would be removed in v2. It is suggests to use the MeterProvider constructor here.
    So, should we keep creating new MeterProvider objects here based on the exporters provided via env variable?

  2. I am assuming that we can provide >1 metric exporters via env configurations, just like trace exporters. Is that correct? I couldn't find a clarification for this in the specification document.

@pichlermarc
Copy link
Member Author

So, should we keep creating new MeterProvider objects here based on the exporters provided via env variable?

If the env var is set, only create a single MeterProvider and add all MetricReaders to that MeterProvider via the constructor option (it's an array). That means that creation of the exporters with auto-paired MetricReaders has to happen before instantiating a MeterProvider.

I am assuming that we can provide >1 metric exporters via env configurations, just like trace exporters. Is that correct? I couldn't find a clarification for this in the specification document.

That's correct. 🙂 The specification reads:

The implementation MAY accept a comma-separated list to enable setting multiple exporters.

so that's applicable to all exporter selection env vars.

@bhaskarbanerjee
Copy link

@pichlermarc is this behavior consistent across python,java, go etc?

@bhaskarbanerjee
Copy link

@pichlermarc would it be possible to have some answers on the above question? I am trying to understand if this by design for all nodejs layer.sdk or all other languages too?

@pichlermarc
Copy link
Member Author

@pichlermarc is this behavior consistent across python,java, go etc?
would it be possible to have some answers on the above question? I am trying to understand if this by design for all nodejs layer.sdk or all other languages too?

I'm not sure what the question is here - the feature is not implemented yet. As with all specification features, the behavior aims to be aligned across all languages.

@bhaskarbanerjee
Copy link

@pichlermarc what I mean to understand is whether python, go, java export metrics by deafult or do they not just like in nodejs. same goes for logs.

@bhaskarbanerjee
Copy link

@pichlermarc if you could please help me get answer on my above question?
@Prashansa-K how far is the PR that you are working on?

@silpamittapalli
Copy link

Hi there @pichlermarc
Following up on Bhaskar's comment above. Is the MetricReader/exporter combination not auto-configured when using @opentelemetry/sdk-node package only? or Is this behavior expected with other language SDKs too? Please clarify.

If this choice was made by design, we would love to understand the reasoning behind this choice.

To expedite progress, we would be happy to claim and/or contribute to this feature if needed. Please let us know @Prashansa-K

@pichlermarc
Copy link
Member Author

pichlermarc commented Jun 14, 2024

Following up on Bhaskar's comment above. Is the MetricReader/exporter combination not auto-configured when using @opentelemetry/sdk-node package only? or Is this behavior expected with other language SDKs too? Please clarify.

I don't know the exact state of all the other language SDKs autoconfigure packages. I know that the specification does not prescribe a default. Posting research here would be very welcome 🙂

If this choice was made by design, we would love to understand the reasoning behind this choice.

It was a conscious decision. As there was no way to set up the metrics exporter, automatically setting up the MeterProvider for it would've been more annoying than helpful until this feature here and #4655 are implemented.

I updated the issue description to reflect that. When implementing this feature do NOT set up a MeterProvider if the OTEL_METRICS_EXPORTER env var is not set. We can revisit that decision when #4655 is implemented.

@bhaskarbanerjee
Copy link

Thanks @pichlermarc for the clarification. As we dig more, shall share the findings here.

@bhaskarbanerjee
Copy link

@Prashansa-K please help us understand how far the PR is. We have needs in this area and are happy to pair up, if that can expedite your contribution.

@bhaskarbanerjee
Copy link

@pichlermarc Is there anything we can do to expedite this?
We are also very willing to cut out a PR for this, if other contributors are busy. Please advice.

@pichlermarc
Copy link
Member Author

@bhaskarbanerjee I don't have any way to reach out to @Prashansa-K, unfortunately.

Looks like there's no response to the above comment and the issue was assigned a little over a month ago.
@bhaskarbanerjee I'd appreciate you opening a PR to implement this feature.

@bhaskarbanerjee
Copy link

Thanks for prompt response @pichlermarc we will get on top of this. cc @silpamittapalli

@haythemtrabelsi117
Copy link

hi, what's the status on this?

@bhaskarbanerjee
Copy link

@haythemtrabelsi117 we got delayed but are now actively working on this. Please expect a PR by end of this month.

@bhaskarbanerjee
Copy link

@pichlermarc @dyladan
I need to make updates to opentelemetry-js/opentelemetry-core (add another supported env var named OTEL_METRICS_EXPORTER) before I can make other updates to the opentelemetry-js/opentelemetry-sdk-node. should I raise 2 separate PRs?

@bhaskarbanerjee
Copy link

@pichlermarc can you please assign this ticket to me?

@pichlermarc
Copy link
Member Author

@bhaskarbanerjee please don't access the core module anymore to get the environment config, we're moving away from that. You can directly use process.env.OTEL_METRICS_EXPORTER and parse that input, then you can do it in the same PR. 🙂

@bhaskarbanerjee
Copy link

Thanks for that info @pichlermarc , will follow that.

@bhaskarbanerjee
Copy link

@pichlermarc I am in process of drafting this PR. I see there is a function for configuring the log provider from env.

The simplest way to solve this would be to add a similar function for configuring the metric provider and then write tests on top of this.

Are you good with approach or do you have another suggestion?

private configureMetricProviderFromEnv(): void {
    const metricsExporterList = process.env.OTEL_METRICS_EXPORTER ?? '';
    const enabledExporters = filterBlanksAndNulls(metricsExporterList.split(','));
    if (enabledExporters.length === 0) {
      diag.info('OTEL_METRICS_EXPORTER is empty. Using default otlp exporter.');
      enabledExporters.push('otlp');
    }

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

    enabledExporters.forEach(exporter => {
      if (exporter === 'otlp') {
        const protocol = (
          process.env.OTEL_EXPORTER_OTLP_METRIC_PROTOCOL ??
          process.env.OTEL_EXPORTER_OTLP_PROTOCOL
        )?.trim();

        switch (protocol) {
          case 'grpc':
            exporters.push(new OTLPGrpcMetricExporter());
            break;
          case 'http/json':
            exporters.push(new OTLPHttpMetricExporter());
            break;
          case 'http/protobuf':
            exporters.push(new OTLPProtoMetricExporter());
            break;
          case undefined:
          case '':
            exporters.push(new OTLPProtoMetricExporter());
            break;
          default:
            diag.warn(
              `Unsupported OTLP metrics protocol: "${protocol}". Using http/protobuf.`
            );
            exporters.push(new OTLPProtoMetricExporter());
        }
      } else if (exporter === 'console') {
        exporters.push(new ConsoleMetricExporter());
      } else if (exporter === 'prometheus') {
        exporters.push(new PrometheusMetricExporter());
      } else {
        diag.warn(
          `Unsupported OTEL_METRICS_EXPORTER value: "${exporter}". Supported values are: otlp, console, prometheus, none.`
        );
      }
    });

    if (exporters.length > 0) {
      exporters.map(exporter => {
          if (exporter instanceof PrometheusMetricExporter) {
            // return a PullBasedReader
            return;
          } else {
            const exportIntervalMillis = process.env.OTEL_METRICS_EXPORTER_INTERVAL ?? 500
            return new PeriodicExportingMetricReader({
              exporter: exporter,
              exportIntervalMillis: exportIntervalMillis
            });
          }
        })
    }
  }

And then add the below lines to end of https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-sdk-node/src/sdk.ts#L212

  else {
      this._meterProviderConfig.reader = this.configureMetricProviderFromEnv()
    }

@bhaskarbanerjee
Copy link

@pichlermarc one more testing question. Apart from Unit testing, what other testing is encouraged and is there a designated process to do so?

@bhaskarbanerjee
Copy link

@pichlermarc any thoughts on the above?

@pichlermarc
Copy link
Member Author

@bhaskarbanerjee yes that sounds good - some suggestion for the code above: you may want to immediately pair the exporter with a PeriodiceExportingMetricReader where applicable - this way you can hold MetricReader instances and you can drop the instanceof check for the prometheus exporter. Other than that this looks good.

@pichlermarc one more testing question. Apart from Unit testing, what other testing is encouraged and is there a designated process to do so?

There's only "unit"-tests in @opentelemetry/sdk-node - if you add tests for the functionality you're adding then that's sufficient. You can check the existing tests - I think similar ones should suffice. FYI: the quality of tests in @opentelemetry/sdk-node is not the best at the moment (they're asserting on lots of internal state of other packages, etc) - if you can come up with higher-quality ones instead of following what's already there, I'd appreciate it. 🙂

@bhaskarbanerjee
Copy link

🆒 Thanks @pichlermarc My PR should be therein a few days. I will try my best with the unit tests.
The current unit tests are failing on other packages. Is there a way to run only the tests for sdk-node package?

 --------------------|---------|----------|---------|---------|--------------------------------------
       File                | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                    
       --------------------|---------|----------|---------|---------|--------------------------------------
       All files           |   89.66 |    82.43 |   98.01 |   89.68 |                                      
        src                |    89.6 |    82.36 |      98 |   89.61 |                                      
         http.ts           |   90.42 |    87.38 |   97.26 |   91.07 | ...,722-734,775,833,839,964-966,1011 
         utils.ts          |   88.92 |    78.59 |     100 |   88.32 | ...0,636-650,670-672,681,750,924,932 
        src/enums          |     100 |      100 |     100 |     100 |                                      
         AttributeNames.ts |     100 |      100 |     100 |     100 |                                      
       --------------------|---------|----------|---------|---------|--------------------------------------
       npm ERR! Lifecycle script `test:cjs` failed with error: 
       npm ERR! Error: command failed 
       npm ERR!   in workspace: @opentelemetry/instrumentation-http@0.53.0 
       npm ERR!   at location: /Users/hkn097/workspace-git/OpenSource/opentelemetry-js/experimental/packages/opentelemetry-instrumentation-http 
       npm ERR! Lifecycle script `test` failed with error: 
       npm ERR! Error: command failed 
       npm ERR!   in workspace: @opentelemetry/instrumentation-http@0.53.0 
       npm ERR!   at location: /Users/hkn097/workspace-git/OpenSource/opentelemetry-js/experimental/packages/opentelemetry-instrumentation-http 
       

    ✖  @opentelemetry/exporter-logs-otlp-proto:test
       > @opentelemetry/exporter-logs-otlp-proto@0.53.0 test
       > nyc mocha 'test/**/*.test.ts' --exclude 'test/browser/**/*.ts'
       
       
        Exception during run: src/platform/node/OTLPLogExporter.ts(27,25): error TS2307: Cannot find module '../../version' or its corresponding type declarations.
       
       ----------|---------|----------|---------|---------|-------------------
       File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
       ----------|---------|----------|---------|---------|-------------------
       All files |       0 |        0 |       0 |       0 |                   
       ----------|---------|----------|---------|---------|-------------------
       npm ERR! Lifecycle script `test` failed with error: 
       npm ERR! Error: command failed 
       npm ERR!   in workspace: @opentelemetry/exporter-logs-otlp-proto@0.53.0 
       npm ERR!   at location: /Users/hkn097/workspace-git/OpenSource/opentelemetry-js/experimental/packages/exporter-logs-otlp-proto 
       

    ✖  @opentelemetry/exporter-logs-otlp-http:test
       > @opentelemetry/exporter-logs-otlp-http@0.53.0 test
       > nyc mocha 'test/**/*.test.ts' --exclude 'test/browser/**/*.ts'
       
       
        Exception during run: test/node/OTLPLogExporter.test.ts(33,25): error TS2307: Cannot find module '../../src/version' or its corresponding type declarations.
       
       ----------|---------|----------|---------|---------|-------------------
       File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
       ----------|---------|----------|---------|---------|-------------------
       All files |       0 |        0 |       0 |       0 |                   
       ----------|---------|----------|---------|---------|-------------------
       npm ERR! Lifecycle script `test` failed with error: 
       npm ERR! Error: command failed 
       npm ERR!   in workspace: @opentelemetry/exporter-logs-otlp-http@0.53.0 
       npm ERR!   at location: /Users/hkn097/workspace-git/OpenSource/opentelemetry-js/experimental/packages/exporter-logs-otlp-http 
       
    ✔  @opentelemetry/instrumentation-grpc:test (15s)

    ✖  @opentelemetry/exporter-metrics-otlp-http:test
       > @opentelemetry/exporter-metrics-otlp-http@0.53.0 test
       > nyc mocha 'test/**/*.test.ts' --exclude 'test/browser/**/*.ts'
       
       
        Exception during run: src/platform/node/OTLPMetricExporter.ts(28,25): error TS2307: Cannot find module '../../version' or its corresponding type declarations.
       
       ----------|---------|----------|---------|---------|-------------------
       File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
       ----------|---------|----------|---------|---------|-------------------
       All files |       0 |        0 |       0 |       0 |                   
       ----------|---------|----------|---------|---------|-------------------
       npm ERR! Lifecycle script `test` failed with error: 
       npm ERR! Error: command failed 
       npm ERR!   in workspace: @opentelemetry/exporter-metrics-otlp-http@0.53.0 
       npm ERR!   at location: /Users/hkn097/workspace-git/OpenSource/opentelemetry-js/experimental/packages/opentelemetry-exporter-metrics-otlp-http 
       

    ✖  @opentelemetry/exporter-trace-otlp-http:test
       > @opentelemetry/exporter-trace-otlp-http@0.53.0 test
       > nyc mocha 'test/**/*.test.ts' --exclude 'test/browser/**/*.ts'
       
       
        Exception during run: src/platform/node/OTLPTraceExporter.ts(21,25): error TS2307: Cannot find module '../../version' or its corresponding type declarations.
       
       ----------|---------|----------|---------|---------|-------------------
       File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
       ----------|---------|----------|---------|---------|-------------------
       All files |       0 |        0 |       0 |       0 |                   
       ----------|---------|----------|---------|---------|-------------------
       npm ERR! Lifecycle script `test` failed with error: 
       npm ERR! Error: command failed 
       npm ERR!   in workspace: @opentelemetry/exporter-trace-otlp-http@0.53.0 
       npm ERR!   at location: /Users/hkn097/workspace-git/OpenSource/opentelemetry-js/experimental/packages/exporter-trace-otlp-http 
       

    ✖  @opentelemetry/exporter-trace-otlp-proto:test
       > @opentelemetry/exporter-trace-otlp-proto@0.53.0 test
       > nyc mocha 'test/**/*.test.ts' --exclude 'test/browser/**/*.ts'
       
       
        Exception during run: test/node/OTLPTraceExporter.test.ts(39,25): error TS2307: Cannot find module '../../src/version' or its corresponding type declarations.
       
       ----------|---------|----------|---------|---------|-------------------
       File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
       ----------|---------|----------|---------|---------|-------------------
       All files |       0 |        0 |       0 |       0 |                   
       ----------|---------|----------|---------|---------|-------------------
       npm ERR! Lifecycle script `test` failed with error: 
       npm ERR! Error: command failed 
       npm ERR!   in workspace: @opentelemetry/exporter-trace-otlp-proto@0.53.0 
       npm ERR!   at location: /Users/hkn097/workspace-git/OpenSource/opentelemetry-js/experimental/packages/exporter-trace-otlp-proto 
       

    ✖  @opentelemetry/otlp-grpc-exporter-base:test
       > @opentelemetry/otlp-grpc-exporter-base@0.53.0 test
       > nyc mocha 'test/**/*.test.ts'
       
       
        Exception during run: test/configuration/otlp-grpc-configuration.test.ts(29,25): error TS2307: Cannot find module '../../src/version' or its corresponding type declarations.
       
       ----------|---------|----------|---------|---------|-------------------
       File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
       ----------|---------|----------|---------|---------|-------------------
       All files |       0 |        0 |       0 |       0 |                   
       ----------|---------|----------|---------|---------|-------------------
       npm ERR! Lifecycle script `test` failed with error: 
       npm ERR! Error: command failed 
       npm ERR!   in workspace: @opentelemetry/otlp-grpc-exporter-base@0.53.0 
       npm ERR!   at location: /Users/hkn097/workspace-git/OpenSource/opentelemetry-js/experimental/packages/otlp-grpc-exporter-base 
       

 ———————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————

 >  Lerna (powered by Nx)   Ran target test for 40 projects (1m)
 
    ✔    28/35 succeeded [0 read from cache]
 
    ✖    7/35 targets failed, including the following:
         - @opentelemetry/instrumentation-http:test
         - @opentelemetry/exporter-logs-otlp-proto:test
         - @opentelemetry/exporter-logs-otlp-http:test
         - @opentelemetry/exporter-metrics-otlp-http:test
         - @opentelemetry/exporter-trace-otlp-http:test
         ...and 2 more...
 
   Hint: Try "nx view-logs" to get structured, searchable errors logs in your browser.
 
sh-3.2$ npm -v
10.5.0
sh-3.2$ node -v
v20.12.0
sh-3.2$ npx lerna run test

@pichlermarc
Copy link
Member Author

@bhaskarbanerjee please read https://github.com/open-telemetry/opentelemetry-js/blob/be1737fc46c1c0fbe75f4d30ce5a196ebeee4c09/CONTRIBUTING.md#development-quick-start and follow the steps there - the steps that are listed there are necessary to setup auto-generated files that are required to make tests pass.

After doing that once you can just run npm run test in ./experimental/opentelemetry-sdk-node/ and it'll just run the tests for that package.

@bhaskarbanerjee
Copy link

Thanks @pichlermarc I will check this out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-contribution This feature/bug is ready to implement pkg:sdk-node triage:accepted This feature has been accepted type:feature A feature with no sub-issues to address
Projects
None yet
Development

No branches or pull requests

5 participants