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

Allow NodeSDK to specify sampler without spanProcessor/exporter #4212

Closed
dvoytenko opened this issue Oct 17, 2023 · 6 comments · Fixed by #4394
Closed

Allow NodeSDK to specify sampler without spanProcessor/exporter #4212

dvoytenko opened this issue Oct 17, 2023 · 6 comments · Fixed by #4394
Labels
bug Something isn't working pkg:sdk-node priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect up-for-grabs Good for taking. Extra help will be provided by maintainers

Comments

@dvoytenko
Copy link
Contributor

What happened?

Steps to Reproduce

const sdk = new NodeSDK({
   sampler: new MySampler(),
});

Expected Result

The specified sampler is used.

Actual Result

Default sampler is initialized.

Additional Details

This is due to this code: the specified sampler is only accepted when the config also contains a spanProcessor or a traceExporter. However, these concepts are not necessarily require each other. It's should be ok to configure one without the other.

OpenTelemetry Setup Code

import { NodeSDK, type NodeSDKConfiguration } from '@opentelemetry/sdk-node';
const sdk = new NodeSDK({
  sampler: new MySampler(), // Never called.
});
sdk.start();


### package.json

```JSON
{
  "dependencies": {
    "@opentelemetry/sdk-node": "^0.44.0"
  }
}

Relevant log output

No response

@dvoytenko dvoytenko added bug Something isn't working triage labels Oct 17, 2023
@dyladan dyladan added priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization and removed triage labels Oct 18, 2023
@dyladan
Copy link
Member

dyladan commented Oct 18, 2023

This means a sampler cannot be configured when using environment variables to set up the export pipeline.

@pichlermarc pichlermarc added triage pkg:sdk-node and removed priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization labels Oct 18, 2023
@dvoytenko
Copy link
Contributor Author

@dyladan Is this a statement for or against supporting sampler without processor/exporter configuration?

@dyladan
Copy link
Member

dyladan commented Oct 25, 2023

It's just a statement of the current state sorry. I should have added more context. I think this is something we should fix, but I think it is a feature request and not a bug.

@dyladan dyladan added feature-request bug Something isn't working priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect and removed bug Something isn't working triage feature-request labels Oct 25, 2023
@dyladan
Copy link
Member

dyladan commented Oct 25, 2023

This actually is a bug the more I think about it. It means the sampler is not being applied correctly.

@pichlermarc pichlermarc added the up-for-grabs Good for taking. Extra help will be provided by maintainers label Nov 9, 2023
@WesleyYue
Copy link

I think this is a duplicate of #4004

@pichlermarc
Copy link
Member

This seems to have been fixed by #4394 - merging the PR did not close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:sdk-node priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect up-for-grabs Good for taking. Extra help will be provided by maintainers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants