feat(opentelemetry-resources): add runtime version information #2727

cuichenli · 2022-01-21T03:51:54Z

Signed-off-by: Cuichen Li [email protected]

Which problem is this PR solving?

Add runtime version information for node resources detector.

In the original issue, it was proposed to also add runtime.name and runtime.description. I feel it does not make much sense for node. But let me know if we want to add it. Following is one example versions output I retrieved:

> process.versions
{
  node: '14.17.0',
  v8: '8.4.371.23-node.63',
  uv: '1.41.0',
  zlib: '1.2.11',
  brotli: '1.0.9',
  ares: '1.17.1',
  modules: '83',
  nghttp2: '1.42.0',
  napi: '8',
  llhttp: '2.1.3',
  openssl: '1.1.1k',
  cldr: '38.1',
  icu: '68.2',
  tz: '2020d',
  unicode: '13.0'
}

Thinking maybe we can add the v8 info to runtime.description? Please kindly advice. Thanks

Fixes #2562

Short description of the changes

Retrieve version info from process

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?

Tested locally it can successfully retrieve the runtime version. Also updated the unit test.

Checklist:

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

Signed-off-by: Cuichen Li <[email protected]>

codecov · 2022-01-21T04:00:49Z

Codecov Report

Merging #2727 (09236f6) into main (ef03d02) will increase coverage by 0.02%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##             main    #2727      +/-   ##
==========================================
+ Coverage   93.39%   93.41%   +0.02%     
==========================================
  Files         159      160       +1     
  Lines        5450     5467      +17     
  Branches     1145     1149       +4     
==========================================
+ Hits         5090     5107      +17     
  Misses        360      360

Impacted Files	Coverage Δ
...ry-resources/src/platform/node/detect-resources.ts	`21.73% <0.00%> (ø)`
...lemetry-resources/src/detectors/BrowserDetector.ts	`100.00% <100.00%> (ø)`
...lemetry-resources/src/detectors/ProcessDetector.ts	`100.00% <100.00%> (ø)`
...ges/opentelemetry-resources/src/detectors/index.ts	`100.00% <100.00%> (ø)`

dyladan · 2022-01-21T21:27:39Z

I believe we need the name in order for the version to be meaningful. The usecase would be many different languages/runtimes sending spans to the backend. How would you know which ones are node (node does not always even appear in the command line)?

I believe we should have:

{
  "process.runtime.name": "nodejs",
  "process.runtime.version": process.versions.node,
  "process.runtime.description": "NodeJS"
}

See also: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/semantic_conventions/process.md#javascript-runtimes

Signed-off-by: Cuichen Li <[email protected]>

cuichenli · 2022-01-24T02:42:23Z

@dyladan thanks, i have hardcoded the runtime name and description.

packages/opentelemetry-resources/src/platform/node/detectors/ProcessDetector.ts

Signed-off-by: Cuichen Li <[email protected]>

packages/opentelemetry-resources/src/platform/browser/detect-resources.ts

packages/opentelemetry-resources/src/platform/browser/detectors/BrowserDetector.ts

packages/opentelemetry-resources/src/platform/node/detectors/ProcessDetector.ts

packages/opentelemetry-resources/src/platform/utils.ts

Signed-off-by: Cuichen Li <[email protected]>

Signed-off-by: Cuichen Li <[email protected]> Co-authored-by: legendecas <[email protected]>

legendecas

LGTM % nits.

legendecas · 2022-01-25T17:05:53Z

packages/opentelemetry-resources/test/detectors/BrowserDetector.test.ts

+
+describe('browserDetector()', () => {
+  beforeEach(() => {
+    global.window = {


sinon.stub should be preferred to create mocks. Also global is not a standard global alias in browser environment, please use globalThis instead.

updated.
tried to

sinon.stub(globalThis, 'window') ....

but got TypeError: Cannot stub non-existent property window

My guess is that these tests are run on the Node.js environment, which is not expected. I'm filed #2739 to fix this. I don't want to block this PR for #2739. So I'm ok with this as is.

packages/opentelemetry-resources/src/platform/browser/detect-resources.ts

Signed-off-by: Cuichen Li <[email protected]>

vmarchaud · 2022-01-26T10:15:28Z

@cuichenli After fixing the build & rebasing the PR should be good to merge !

Signed-off-by: Cuichen Li <[email protected]>

…-runtime-version Signed-off-by: Cuichen Li <[email protected]>

cuichenli · 2022-01-26T16:04:49Z

@vmarchaud I have fixed the issue, can you please re-trigger the build? thanks

dyladan · 2022-01-26T16:38:02Z

@legendecas is this PR blocked on your spec PR or no?

legendecas · 2022-01-26T16:52:32Z

@dyladan yeah, it will be great to see how open-telemetry/opentelemetry-specification#2290 goes after the review. Most likely there aren't many things to change since the spec PR just writes down what we are doing in this PR, except that the spec PR removed process.runtime.description since it adds no additional information.

packages/opentelemetry-resources/src/platform/browser/detectors/BrowserDetector.ts

legendecas · 2022-03-02T02:46:57Z

@cuichenli I'm so sorry for this to wait such a long time. I think open-telemetry/opentelemetry-specification#2290 is going to be merged unchanged. But it took a long time already and I'd believe we should get things rolling and this PR should not be blocked. Would you mind resolving the conflicts so that we can get this PR merged? Thank you very much.

…-runtime-version

cuichenli · 2022-03-02T02:56:56Z

@legendecas thanks! i have updated the branch.

legendecas

Thank you for your work! I think the PR is almost complete once we resolve these two issues.

legendecas · 2022-03-02T03:20:29Z

packages/opentelemetry-resources/src/platform/browser/detectors/BrowserDetector.ts

@@ -0,0 +1,61 @@
+/*


The detectors were moved to the src/detectors folder so that we can have a consistent export on both Node.js and Web environments. And detector.detect will do nothing if the environment is not the expected one, like https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-resources/src/detectors/ProcessDetector.ts#L30.

Could you move the BrowserDetector to src/detectors too so that it can be exported unconditionally?

legendecas · 2022-03-02T03:25:45Z

packages/opentelemetry-resources/src/platform/utils.ts

+  resources.forEach(resource => {
+    // Print only populated resources
+    if (Object.keys(resource.attributes).length > 0) {
+      const resourceDebugString = util.inspect(resource.attributes, {


util.inspect is not available on the Web, that's why it is located in the src/platform/node so that it won't be bundled in the web applications. Moving this API out of src/platform/node makes it unusable on Web environments. I think this change could be separated into another PR to make detectResource Web compatible.

i will file an issue for this and work on it in another pr. thanks!

packages/opentelemetry-resources/src/platform/browser/detectors/BrowserDetector.ts

dyladan · 2022-03-02T15:52:20Z

Spec PR merged which means when this is ready it can be merged

Signed-off-by: Cuichen Li <[email protected]>

cuichenli · 2022-03-03T03:55:24Z

@legendecas have updated based on the comments. please take another look. thanks

legendecas · 2022-03-03T17:49:05Z

packages/opentelemetry-resources/test/detectors/browser/BrowserDetector.test.ts

+describe('browserDetector()', () => {
+  beforeEach(() => {
+    (globalThis.window as {}) = {};
+    sinon.stub(globalThis, 'window').value({


The test is failing because globalThis.window in the browser environment is not configurable or writable. You may need to stub the property you need instead of stub the whole "window" object.

Test isn't failing...

fixed. but it took me a while to realize we now have describeNode and describeBrowser 🤦

legendecas · 2022-03-03T17:50:04Z

packages/opentelemetry-resources/test/detectors/node/BrowserDetector.test.ts

+
+describe('browserDetector()', () => {
+  beforeEach(() => {
+    (globalThis.window as {}) = {};


Since this file will be tested in the Node.js environment only, we don't need to mock the globalThis.window anymore.

Signed-off-by: Cuichen Li <[email protected]>

…o add-runtime-version

legendecas · 2022-03-04T15:56:00Z

Thank you for your work!

cuichenli added 2 commits January 21, 2022 11:28

feat(opentelemetry-resources): add runtime version information

7ce4389

Signed-off-by: Cuichen Li <[email protected]>

test(opentelemetry-resources): update unit test

7bd392f

Signed-off-by: Cuichen Li <[email protected]>

cuichenli requested a review from a team January 21, 2022 03:51

vmarchaud added the enhancement New feature or request label Jan 23, 2022

feat(opentelemetry-resources): hardcode the runtime name and description

8607046

Signed-off-by: Cuichen Li <[email protected]>

vmarchaud reviewed Jan 24, 2022

View reviewed changes

packages/opentelemetry-resources/src/platform/node/detectors/ProcessDetector.ts Outdated Show resolved Hide resolved

feat(opentelemetry-resources): add browser detector

1827b95

Signed-off-by: Cuichen Li <[email protected]>

dyladan reviewed Jan 24, 2022

View reviewed changes

packages/opentelemetry-resources/src/platform/browser/detect-resources.ts Outdated Show resolved Hide resolved

packages/opentelemetry-resources/src/platform/browser/detectors/BrowserDetector.ts Outdated Show resolved Hide resolved

legendecas mentioned this pull request Jan 25, 2022

Define JavaScript runtimes semantic conventions open-telemetry/opentelemetry-specification#2290

Merged

legendecas reviewed Jan 25, 2022

View reviewed changes

packages/opentelemetry-resources/src/platform/node/detectors/ProcessDetector.ts Outdated Show resolved Hide resolved

packages/opentelemetry-resources/src/platform/utils.ts Outdated Show resolved Hide resolved

cuichenli and others added 3 commits January 25, 2022 22:24

fix: strange import and export

304edad

Signed-off-by: Cuichen Li <[email protected]>

fix: improve type

abcf4a0

Signed-off-by: Cuichen Li <[email protected]> Co-authored-by: legendecas <[email protected]>

fix: improve runtime description

ee76a6c

Signed-off-by: Cuichen Li <[email protected]> Co-authored-by: legendecas <[email protected]>

cuichenli requested review from legendecas, vmarchaud and dyladan January 25, 2022 14:27

legendecas approved these changes Jan 25, 2022

View reviewed changes

dyladan approved these changes Jan 25, 2022

View reviewed changes

cuichenli added 3 commits January 26, 2022 10:06

test: fix unit test

d25b453

Signed-off-by: Cuichen Li <[email protected]>

test: improve stub

3d36a7c

Signed-off-by: Cuichen Li <[email protected]>

docs: remove unnecessary inline docs

caea930

Signed-off-by: Cuichen Li <[email protected]>

vmarchaud approved these changes Jan 26, 2022

View reviewed changes

cuichenli added 2 commits January 26, 2022 21:34

fix: hack the type error

b8e73c6

Signed-off-by: Cuichen Li <[email protected]>

Merge branch 'main' of github.com:cuichenli/opentelemetry-js into add…

d78d714

…-runtime-version Signed-off-by: Cuichen Li <[email protected]>

MSNev reviewed Jan 26, 2022

View reviewed changes

packages/opentelemetry-resources/src/platform/browser/detectors/BrowserDetector.ts Outdated Show resolved Hide resolved

vmarchaud added the spec-needed label Jan 31, 2022

legendecas added the waiting-for-spec label Feb 7, 2022

legendecas removed the waiting-for-spec label Mar 2, 2022

Merge branch 'main' of github.com:cuichenli/opentelemetry-js into add…

478cab0

…-runtime-version

legendecas reviewed Mar 2, 2022

View reviewed changes

cuichenli added 3 commits March 3, 2022 11:42

chore: move BrowserDetector to detectors directory

1f1ca09

Signed-off-by: Cuichen Li <[email protected]>

chore: move detectResources back to different platform

c749614

Signed-off-by: Cuichen Li <[email protected]>

Merge branch 'main' into add-runtime-version

508fce0

legendecas reviewed Mar 3, 2022

View reviewed changes

cuichenli added 2 commits March 4, 2022 09:43

test: use describeNode and describeBrowser accordingly

abb3d3a

Signed-off-by: Cuichen Li <[email protected]>

Merge branch 'main' of github.com:open-telemetry/opentelemetry-js int…

09236f6

…o add-runtime-version

legendecas approved these changes Mar 4, 2022

View reviewed changes

legendecas merged commit a901732 into open-telemetry:main Mar 4, 2022

dyladan mentioned this pull request Mar 16, 2022

chore: release proposal 1.1.0 #2838

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(opentelemetry-resources): add runtime version information #2727

feat(opentelemetry-resources): add runtime version information #2727

cuichenli commented Jan 21, 2022

codecov bot commented Jan 21, 2022 •

edited

Loading

dyladan commented Jan 21, 2022

cuichenli commented Jan 24, 2022

legendecas left a comment

legendecas Jan 25, 2022

cuichenli Jan 26, 2022

legendecas Jan 26, 2022

vmarchaud commented Jan 26, 2022

cuichenli commented Jan 26, 2022

dyladan commented Jan 26, 2022

legendecas commented Jan 26, 2022

legendecas commented Mar 2, 2022

cuichenli commented Mar 2, 2022

legendecas left a comment

legendecas Mar 2, 2022

legendecas Mar 2, 2022

cuichenli Mar 3, 2022

dyladan commented Mar 2, 2022

cuichenli commented Mar 3, 2022

legendecas Mar 3, 2022 •

edited

Loading

dyladan Mar 3, 2022

cuichenli Mar 4, 2022

legendecas Mar 3, 2022

cuichenli Mar 4, 2022

legendecas commented Mar 4, 2022

feat(opentelemetry-resources): add runtime version information #2727

feat(opentelemetry-resources): add runtime version information #2727

Conversation

cuichenli commented Jan 21, 2022

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Jan 21, 2022 • edited Loading

Codecov Report

dyladan commented Jan 21, 2022

cuichenli commented Jan 24, 2022

legendecas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmarchaud commented Jan 26, 2022

cuichenli commented Jan 26, 2022

dyladan commented Jan 26, 2022

legendecas commented Jan 26, 2022

legendecas commented Mar 2, 2022

cuichenli commented Mar 2, 2022

legendecas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan commented Mar 2, 2022

cuichenli commented Mar 3, 2022

legendecas Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legendecas commented Mar 4, 2022

codecov bot commented Jan 21, 2022 •

edited

Loading

legendecas Mar 3, 2022 •

edited

Loading