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

feat(node-sdk): add serviceName config option #2867

Conversation

naseemkullah
Copy link
Member

@naseemkullah naseemkullah commented Mar 26, 2022

Which problem is this PR solving?

Fixes #2799

configure service name directly in the node SDK config

Short description of the changes

add optional serviceName option in node SDK constructor, which behind the scenes creates another resource that is merged with any one that may have been passed in. this occurs after autoDetectResources so that the in code config set service name trumps any env set service name

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

  • unit tests

Checklist:

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

@naseemkullah naseemkullah requested a review from a team March 26, 2022 20:12
@codecov
Copy link

codecov bot commented Mar 26, 2022

Codecov Report

Merging #2867 (495f5cf) into main (3cc40d7) will decrease coverage by 0.54%.
The diff coverage is n/a.

❗ Current head 495f5cf differs from pull request most recent head c80bde4. Consider uploading reports for the commit c80bde4 to get more accurate results

@@            Coverage Diff             @@
##             main    #2867      +/-   ##
==========================================
- Coverage   92.64%   92.09%   -0.55%     
==========================================
  Files         187       82     -105     
  Lines        6169     2405    -3764     
  Branches     1303      520     -783     
==========================================
- Hits         5715     2215    -3500     
+ Misses        454      190     -264     
Impacted Files Coverage Δ
...s/opentelemetry-core/src/platform/node/sdk-info.ts 0.00% <0.00%> (-100.00%) ⬇️
...opentelemetry-core/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...pentelemetry-core/src/platform/node/performance.ts 0.00% <0.00%> (-100.00%) ⬇️
...emetry-api-metrics/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...lemetry-resources/src/detectors/ProcessDetector.ts 31.81% <0.00%> (-68.19%) ⬇️
...perimental/packages/otlp-exporter-base/src/util.ts 79.41% <0.00%> (-17.65%) ⬇️
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 84.48% <0.00%> (-15.52%) ⬇️
...lemetry-resources/src/detectors/BrowserDetector.ts 93.33% <0.00%> (-6.67%) ⬇️
...imental/packages/opentelemetry-sdk-node/src/sdk.ts
...dk-metrics-base/src/state/MetricStorageRegistry.ts
... and 103 more

@naseemkullah naseemkullah force-pushed the node-sdk-semantic-attributes-config branch 9 times, most recently from b1506a7 to 3f92f1f Compare March 27, 2022 05:23
@naseemkullah naseemkullah force-pushed the node-sdk-semantic-attributes-config branch 2 times, most recently from b615465 to 4214cb9 Compare May 26, 2022 02:40
@naseemkullah
Copy link
Member Author

OK, now back to original proposal of just adding a serviceName config option 👍

@naseemkullah naseemkullah force-pushed the node-sdk-semantic-attributes-config branch from 4214cb9 to 363c665 Compare May 26, 2022 03:15
@naseemkullah naseemkullah changed the title feat: add semantic resource attributes in node sdk config feat(node-sdk): add serviceName config option May 28, 2022
@naseemkullah naseemkullah force-pushed the node-sdk-semantic-attributes-config branch from 363c665 to 15cf5b6 Compare May 28, 2022 15:13
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM

experimental/CHANGELOG.md Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-sdk-node/src/types.ts Outdated Show resolved Hide resolved
@naseemkullah naseemkullah force-pushed the node-sdk-semantic-attributes-config branch from 15cf5b6 to a95577c Compare June 16, 2022 12:31
@naseemkullah naseemkullah force-pushed the node-sdk-semantic-attributes-config branch from a95577c to c89cdd9 Compare June 16, 2022 12:33
@legendecas legendecas merged commit fcb9565 into open-telemetry:main Jun 20, 2022
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.

NodeSDK should take a serviceName prop
6 participants