Skip to content

feat(node-sdk): add spanProcessors option - #4454

Merged
naseemkullah merged 1 commit into
open-telemetry:mainfrom
naseemkullah:node-sdk-span-processors
Feb 5, 2024
Merged

feat(node-sdk): add spanProcessors option#4454
naseemkullah merged 1 commit into
open-telemetry:mainfrom
naseemkullah:node-sdk-span-processors

Conversation

@naseemkullah

@naseemkullah naseemkullah commented Jan 31, 2024

Copy link
Copy Markdown
Member

Which problem is this PR solving?

Since registered span processors are chained together via that active multispanprocessor, it makes sense to expose the option as an array of span processors. As such this change also deprecates the spanProcessor option in favor of the new, more flexiblespanProcessors option.

use case: suppose you are happy with the default batchspan processor, but rather not override any of its hooks and just add a subsequent processor that does something e.g. extracts baggage and adds the baggage entries to spans (side note: an off the shelf baggage extractor span processor might be nice!).

Fixes # (issue)

Short description of the changes

add a spanProcessors config option. convert the original/deprecated spanProcessor config to an array of one if used. we now loop over the spanProcessors and register them to the tracer provider.

n.b. if someone so happens to set both spanProcessor and spanProcessors it will default to using the latter, non deprecated option

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

  • The main span processor test now tests that an array is passed in successfully.

Checklist:

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

@naseemkullah
naseemkullah requested a review from a team January 31, 2024 22:04
@codecov

codecov Bot commented Jan 31, 2024

Copy link
Copy Markdown

Codecov Report

Merging #4454 (c8c3072) into main (2df6310) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4454      +/-   ##
==========================================
+ Coverage   92.37%   92.43%   +0.05%     
==========================================
  Files         304      330      +26     
  Lines        8682     9504     +822     
  Branches     1831     2026     +195     
==========================================
+ Hits         8020     8785     +765     
- Misses        662      719      +57     
Files Coverage Δ
...imental/packages/opentelemetry-sdk-node/src/sdk.ts 93.18% <100.00%> (+0.38%) ⬆️

... and 40 files with indirect coverage changes

@naseemkullah
naseemkullah force-pushed the node-sdk-span-processors branch from 921c71e to af7b892 Compare January 31, 2024 22:07
@naseemkullah
naseemkullah marked this pull request as draft January 31, 2024 22:08
@naseemkullah
naseemkullah force-pushed the node-sdk-span-processors branch 5 times, most recently from 67e37ab to 9011815 Compare February 1, 2024 01:46
@naseemkullah
naseemkullah marked this pull request as ready for review February 1, 2024 01:46
@naseemkullah

Copy link
Copy Markdown
Member Author

This is now ready for review. As per today's SIG this adds the spanProcessors (array) option to node-sdk

@pichlermarc pichlermarc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @naseemkullah 🙂
Looks good % nits

Comment thread experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts Outdated
Comment thread experimental/CHANGELOG.md Outdated
Since span processors can be chained together, it makes sense to expose
this option as an array of span processors. This change also deprecates
the `spanProcessor` option in favor of `spanProcessors`.
@naseemkullah
naseemkullah force-pushed the node-sdk-span-processors branch from 8926483 to c8c3072 Compare February 1, 2024 15:19
@naseemkullah

Copy link
Copy Markdown
Member Author

Thanks @pichlermarc!

@hectorhdzg

Copy link
Copy Markdown
Member

Related to #4451, it would be good to add LogProcessors as well.

@naseemkullah
naseemkullah merged commit 72c60f1 into open-telemetry:main Feb 5, 2024
@naseemkullah
naseemkullah deleted the node-sdk-span-processors branch February 5, 2024 12:05
valerybugakov added a commit to sourcegraph/cody-public-snapshot that referenced this pull request Feb 27, 2024
…3270)

Currently, `NodeSDK` does not support multiple-span processors and exporters. This functionality [was added](open-telemetry/opentelemetry-js#4454) three weeks ago and will be included in [the following experimental release](open-telemetry/opentelemetry-js#4504). To fix the problem without waiting for the release, I migrated this functionality to lower-level primitives, where we have complete control over the number of exporters.
steveyegge pushed a commit to sourcegraph/cody-public-snapshot that referenced this pull request Mar 13, 2024
…3270)

Currently, `NodeSDK` does not support multiple-span processors and exporters. This functionality [was added](open-telemetry/opentelemetry-js#4454) three weeks ago and will be included in [the following experimental release](open-telemetry/opentelemetry-js#4504). To fix the problem without waiting for the release, I migrated this functionality to lower-level primitives, where we have complete control over the number of exporters.
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants