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

tracer provider always attempts to read exporter from env #3449

Closed
blumamir opened this issue Nov 29, 2022 · 11 comments
Closed

tracer provider always attempts to read exporter from env #3449

blumamir opened this issue Nov 29, 2022 · 11 comments
Assignees
Labels
bug Something isn't working priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization sdk:traces Issues and PRs related to the Traces SDK

Comments

@blumamir
Copy link
Member

What happened?

Steps to Reproduce

seems that BasicTracerProvider constructor always attempts to _buildExporterFromEnv here.

This get's executed even when the user supplies his own exporter in code, and there is no need to search the env, for example, when using NodeSDK traceExporter config option.

The _buildExporterFromEnv function will output error to it's diag logger, which is falsy as the workflow is valid.

Expected Result

No errors in diag channel when user initialize the sdk/tracer provider in a valid, documented pattern.

Actual Result

There is an error message in diag:

Exporter "otlp" requested through environment variable is unavailable.

Additional Details

I think that BasicTracerProvider should only attempt to initialize components from env when the user did not set them.

OpenTelemetry Setup Code

import { Resource } from '@opentelemetry/resources';
import { NodeSDK } from '@opentelemetry/sdk-node';
import { ConsoleSpanExporter } from '@opentelemetry/sdk-trace-base';
import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions';
import { diag, DiagConsoleLogger, DiagLogLevel } from '@opentelemetry/api';

import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';

diag.setLogger(new DiagConsoleLogger(), DiagLogLevel.ALL);

const sdk = new NodeSDK({
    resource: new Resource({
        [SemanticResourceAttributes.SERVICE_NAME]: 'amir-testing',
    }),
    traceExporter: new ConsoleSpanExporter(),
    instrumentations: [new HttpInstrumentation()],
});

sdk.start();

package.json

{
  "name": "playground",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "scripts": {
    "start": "ts-node-dev --respawn index.ts"
  },
  "dependencies": {
    "@opentelemetry/api": "^1.3.0",
    "@opentelemetry/exporter-trace-otlp-grpc": "^0.34.0",
    "@opentelemetry/instrumentation-dns": "^0.31.0",
    "@opentelemetry/instrumentation-http": "^0.34.0",
    "@opentelemetry/instrumentation-net": "^0.31.0",
    "@opentelemetry/resources": "^1.8.0",
    "@opentelemetry/sdk-node": "^0.34.0",
    "@opentelemetry/sdk-trace-base": "^1.8.0",
    "@opentelemetry/semantic-conventions": "^1.8.0",
    "axios": "^1.2.0"
  },
  "devDependencies": {
    "ts-node-dev": "^2.0.0",
    "typescript": "^4.9.3"
  }
}

Relevant log output

➜  playground yarn start
yarn run v1.22.19
$ ts-node-dev --respawn index.ts
[INFO] 12:44:17 ts-node-dev ver. 2.0.0 (using ts-node ver. 10.9.1, typescript ver. 4.9.3)
@opentelemetry/api: Registered a global for diag v1.3.0.
opentelemetry initialized
@opentelemetry/instrumentation-http Applying patch for [email protected]
@opentelemetry/instrumentation-http Applying patch for [email protected]
before
@opentelemetry/instrumentation-http https instrumentation outgoingRequest
@opentelemetry/instrumentation-http http.ClientRequest return request
EnvDetector found resource. Resource { attributes: {} }
ProcessDetector found resource. Resource {
  attributes: {
    'process.pid': 75141,
    'process.executable.name': '/usr/local/bin/node',
    'process.command': 'index.ts',
    'process.command_line': '/usr/local/bin/node index.ts',
    'process.runtime.version': '16.16.0',
    'process.runtime.name': 'nodejs',
    'process.runtime.description': 'Node.js'
  }
}
Trace: {
  'process.command': 'index.ts',
  'process.command_line': '/usr/local/bin/node index.ts',
  'process.executable.name': '/usr/local/bin/node',
  'process.pid': 75141,
  'process.runtime.description': 'Node.js',
  'process.runtime.name': 'nodejs',
  'process.runtime.version': '16.16.0'
}
    at DiagConsoleLogger.verbose (/Users/amirblum/repos/playground/node_modules/@opentelemetry/api/src/diag/consoleLogger.ts:49:28)
    at DiagAPI.verbose (/Users/amirblum/repos/playground/node_modules/@opentelemetry/api/src/api/diag.ts:60:32)
    at /Users/amirblum/repos/playground/node_modules/@opentelemetry/resources/src/platform/node/detect-resources.ts:71:12
    at Array.forEach (<anonymous>)
    at logResources (/Users/amirblum/repos/playground/node_modules/@opentelemetry/resources/src/platform/node/detect-resources.ts:62:13)
    at detectResources (/Users/amirblum/repos/playground/node_modules/@opentelemetry/resources/src/platform/node/detect-resources.ts:47:3)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at NodeSDK.detectResources (/Users/amirblum/repos/playground/node_modules/@opentelemetry/sdk-node/src/sdk.ts:174:22)
    at NodeSDK.start (/Users/amirblum/repos/playground/node_modules/@opentelemetry/sdk-node/src/sdk.ts:187:7)
{ exporterName: 'otlp' }
Exporter "otlp" requested through environment variable is unavailable.
@opentelemetry/api: Registered a global for trace v1.3.0.
@opentelemetry/api: Registered a global for context v1.3.0.
@opentelemetry/api: Registered a global for propagation v1.3.0.
@blumamir blumamir added bug Something isn't working triage labels Nov 29, 2022
@dyladan dyladan added priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization and removed triage labels Nov 30, 2022
@legendecas legendecas added the sdk:traces Issues and PRs related to the Traces SDK label Dec 2, 2022
@legendecas
Copy link
Member

Might be related: #3422

@svetlanabrennan
Copy link
Contributor

Should I just add some logic that checks if a user has an exporter defined, then don't run _buildExporterFromEnv

@legendecas
Copy link
Member

Should I just add some logic that checks if a user has an exporter defined, then don't run _buildExporterFromEnv

I'm not sure about the feasible way to achieve this without breaking, as the _buildExporterFromEnv is called from the constructor of BasicTracerProvider. Also, the first call to BasicTracerProvider.addSpanProcessor disables the exporter enabled from the environ. Maybe @vmarchaud can chime in here?

Anyway, if you have any idea about this, please go ahead!

@svetlanabrennan
Copy link
Contributor

svetlanabrennan commented Dec 7, 2022

This pr changed OTEL_TRACES_EXPORTER from none to otlp which is causing this issue. What if we just changed OTEL_TRACES_EXPORTER to "" and update _buildExporterFromEnv to check if the exporterName is equal to "" as well which will return the original functionality of _buildExporterFromEnv to just return when an exporter was not set (previously it would only return when OTEL_TRACES_EXPORTER was set to none).

_buildExporterFromEnv() {
        const exporterName = (0, core_1.getEnv)().OTEL_TRACES_EXPORTER;
        if (exporterName === 'none' || exporterName === '')
            return;
        const exporter = this._getSpanExporter(exporterName);
        if (!exporter) {
            api_1.diag.error(`Exporter "${exporterName}" requested through environment variable is unavailable.`);
        }
        return exporter;
    }

And then I can update the TracerProviderWithEnvExporters class to set otlp as the default exporter when a user doesn't set an exporter in the config when using the NodeSDK package and OTEL_TRACES_EXPORTER is set to "" (TracerProviderWithEnvExporters is only used by NodeSDK).

This solution would just mean that a user will get the default otlp exporter if they set OTEL_TRACES_EXPORTER to an empty string when using the NodeSDK package. The spec doesn't say anything about what to do if a user sets OTEL_TRACES_EXPORTER to an empty string but the spec says to use otlp as the default. Maybe we can warn the user that OTEL_TRACES_EXPORTER is set to "" and we will use the default otlp exporter instead and they have to use none to not auto configure an exporter for traces.

@svetlanabrennan
Copy link
Contributor

@blumamir I've opened a pr with a potential soution to this issue. Would love your thoughts on it. :)

#3492

@mat-rumian
Copy link

Hello @svetlanabrennan,
I faced the same issue on my side: Exporter "otlp" requested through environment variable is unavailable.

Please see the packages:

├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
└── @opentelemetry/[email protected]

Here you will find an init code.

@AkselAllas
Copy link
Contributor

AkselAllas commented Feb 13, 2023

This issue should be closed as it's released in newest version? 🤔

@mat-rumian
Copy link

@AkselAllas I think it is not, I took the latest packages and issue still exists.

@AkselAllas
Copy link
Contributor

Issue is still present. Can confirm

@david-luna
Copy link
Contributor

@blumamir this may be solved in #5138

  • tracer provider does accept now a list of processors in the constructor. If processors are present no exporter is built from env.
  • if we get the exporter/processor from env it will be shut down if a new processor is added via addSpanProcessor API.

@david-luna
Copy link
Contributor

I've noticed this is the same as #3422

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization sdk:traces Issues and PRs related to the Traces SDK
Projects
None yet
Development

No branches or pull requests

7 participants