fix(host-metrics): macOS bundling fix #2071

Netail · 2024-03-30T00:00:24Z

Which problem is this PR solving?

Fix bundling

Short description of the changes

Use specific file, instead of bundling all file of package

linux-foundation-easycla · 2024-03-30T00:00:29Z

The committers listed above are authorized under a signed CLA.

✅ login: Netail / name: Maikel (03e2237, 2ac772e, 1106e97, 9cbe462, a8196e4, 343733e, 83c58a3, d1c1fca, 2484ef6, ce92ffe, 9f81a2d, 63cc39d, ae578fc, 38b80b1, 2c6d8cf, 769de9e, 08506a4, 744ea1d, b642766, 304dc98, e818ba5, 60e0933, a2aaa4b, 65aabe7, 9c30315, fd50fe5, 4fd1f8f, cc8aafe, 8f65520, a3b898f, fc23083, a3f9a61, 5338456, 72ba6f9, 4c0c3c9, 06ac5e7, 5922334, f0a7187, 5e6370a, 9c14d64, deb3257, 3df9163, 79555fc, c0fbc8b, f675811)

packages/opentelemetry-host-metrics/src/stats/si.ts

Netail · 2024-04-02T14:41:14Z

CI/CD should be fixed now :) (btw; npm install fails with Node 20 due to grpc)

david-luna · 2024-04-04T11:45:07Z

Hi @Netail

Thank you for contributing to OTEL :)

We can leverage this PR to help effort the group is doing to update the semantic conventions. Currently we're in a step we need to use the exported strings as you did in this PR and also document which version of the semantic conventions is using.

Would you please add a section in the README file with this info. Here is an example of what I'm referring to.
https://github.com/open-telemetry/opentelemetry-js-contrib/pull/2039/files#diff-24b1cca18e57b052dc9ad991566c9e6cc4a10a47d8528e347ddda139d1ce705e

Netail · 2024-04-04T12:07:55Z

Hi @Netail

Thank you for contributing to OTEL :)

We can leverage this PR to help effort the group is doing to update the semantic conventions. Currently we're in a step we need to use the exported strings as you did in this PR and also document which version of the semantic conventions is using.

Would you please add a section in the README file with this info. Here is an example of what I'm referring to. https://github.com/open-telemetry/opentelemetry-js-contrib/pull/2039/files#diff-24b1cca18e57b052dc9ad991566c9e6cc4a10a47d8528e347ddda139d1ce705e

Hi @david-luna
So if I understand correctly, I have to add all of https://github.com/Netail/opentelemetry-js-contrib/blob/fix/host-metrics-bundling/packages/opentelemetry-host-metrics/src/enum.ts to https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-semantic-conventions, use the package in host-metrics (replacing the package based ones) and adding the ones used from the semantic-conventions package in the README with the table template?

Netail · 2024-04-04T13:23:51Z

Just checked regarding opentelemetry-semantic-conventions package; it fetches the specifications from opentelemetry-specification, but the specifications folder got removed in v1.21.0, so the generator is broken... :(

david-luna · 2024-04-04T13:42:44Z

Hi @david-luna So if I understand correctly, I have to add all of https://github.com/Netail/opentelemetry-js-contrib/blob/fix/host-metrics-bundling/packages/opentelemetry-host-metrics/src/enum.ts to https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-semantic-conventions, use the package in host-metrics (replacing the package based ones) and adding the ones used from the semantic-conventions package in the README with the table template?

Semantic conventions already has these enums but the package is not updated yet so no need to contribute to https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-semantic-conventions. Actually this package is generated from the models in https://github.com/open-telemetry/semantic-conventions

We need to inform which version of semantic conventions model (not package) host metrics is using and list them. In this case the package uses v1.24.0 of the model

You could add a section with this message

## Semantic Conventions

This package uses Semantic Conventions [Version 1.24.0](https://github.com/open-telemetry/semantic-conventions/tree/v1.24.0/docs/http). As for now the Semantic Conventions
are bundled in this package but eventually will be imported from `@opentelemetry/semantic-conventions` package when it is updated to latest version.
Ref: [opentelemetry-js/issues/4235](https://github.com/open-telemetry/opentelemetry-js/issues/4235)

Attributes collected:

| Attribute          | Short Description    | Notes                   |
| ------------------ | -------------------- | ----------------------- |
| `system.cpu.state` | The state of the CPU | Key: `SYSTEM_CPU_STATE` |
| ... other attributes  |

Netail · 2024-04-04T13:47:14Z

Hi @david-luna So if I understand correctly, I have to add all of https://github.com/Netail/opentelemetry-js-contrib/blob/fix/host-metrics-bundling/packages/opentelemetry-host-metrics/src/enum.ts to https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-semantic-conventions, use the package in host-metrics (replacing the package based ones) and adding the ones used from the semantic-conventions package in the README with the table template?

Semantic conventions already has these enums but the package is not updated yet so no need to contribute to https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-semantic-conventions. Actually this package is generated from the models in https://github.com/open-telemetry/semantic-conventions

We need to inform which version of semantic conventions model (not package) host metrics is using and list them. In this case the package uses v1.24.0 of the model

system https://github.com/open-telemetry/semantic-conventions/blob/v1.24.0/docs/system/system-metrics.md

process https://github.com/open-telemetry/semantic-conventions/blob/v1.24.0/docs/system/process-metrics.md

You could add a section with this message
## Semantic Conventions

This package uses Semantic Conventions [Version 1.24.0](https://github.com/open-telemetry/semantic-conventions/tree/v1.24.0/docs/http). As for now the Semantic Conventions
are bundled in this package but eventually will be imported from `@opentelemetry/semantic-conventions` package when it is updated to latest version.
Ref: [opentelemetry-js/issues/4235](https://github.com/open-telemetry/opentelemetry-js/issues/4235)

Attributes collected:

| Attribute          | Short Description    | Notes                   |
| ------------------ | -------------------- | ----------------------- |
| `system.cpu.state` | The state of the CPU | Key: `SYSTEM_CPU_STATE` |
| ... other attributes  |

Shall I pull up a PR which generates the semantic concentions from the new repo?

Netail · 2024-04-04T15:49:32Z

Updated the generator here: open-telemetry/opentelemetry-js#4606

david-luna · 2024-04-04T18:21:10Z

Updated the generator here: open-telemetry/opentelemetry-js#4606

this is indeed one of the tasks to be done for updating semantic conventions but since it a big leap in versions we expect breaking changes. This was discussed in the JavaScript SIG and planned. The plan is summarised and tracked in open-telemetry/opentelemetry-js#4572

the PR looks good but 1st we need to prepare the packages in contrib to be ready for the change. You can help us with these preparation tasks :)

Also you can join the SIG to discuss this topic and others. Here is the doc with the MoMs and also with the info to join https://docs.google.com/document/d/1tCyoQK49WVcE-x8oryZOTTToFm7sIeUhxFPm9g-qL1k/edit

Netail · 2024-04-04T19:46:49Z

Updated the generator here: open-telemetry/opentelemetry-js#4606

this is indeed one of the tasks to be done for updating semantic conventions but since it a big leap in versions we expect breaking changes. This was discussed in the JavaScript SIG and planned. The plan is summarised and tracked in open-telemetry/opentelemetry-js#4572

the PR looks good but 1st we need to prepare the packages in contrib to be ready for the change. You can help us with these preparation tasks :)

Also you can join the SIG to discuss this topic and others. Here is the doc with the MoMs and also with the info to join https://docs.google.com/document/d/1tCyoQK49WVcE-x8oryZOTTToFm7sIeUhxFPm9g-qL1k/edit

What kind of preparation do all the packages need?

Updated open-telemetry/opentelemetry-js#4606 to include v1.25.0 Semantic Conventions. As all packages within the same repo have a different version specified, we can merge that PR, and create different PRs for the packages who use the semantic-conventions package

Added the semantic conventions table to the README

packages/opentelemetry-host-metrics/README.md

packages/opentelemetry-host-metrics/src/BaseMetrics.ts

david-luna · 2024-04-05T11:44:58Z

packages/opentelemetry-host-metrics/global.d.ts

I guess with the change suggested by @pichlermarc in ./src/stats/si.ts

- import { networkStats } from 'systeminformation/lib/network'; + import { networkStats } from 'systeminformation';

this file is no needed. I see the .d.ts file from systeminformation exporting networkStats function already

When I use import { networkStats } from 'systeminformation';, it still tries to import the underlying cpu.js file, thus throwing an error on macOS. :( By targeting the file directly it won't.

The systeminformation/lib/cpu.js file tries to import osx-temperature-sensor for macOS, which you additionally have to install for macOS temperatures as it's not included in the dependencies, however osx-temp-sensor package breaks something else. But the cpu.js file isn't even used for host-metrics, so installing this package besides systeminformation is unecessary

david-luna · 2024-04-05T11:47:04Z

What kind of preparation do all the packages need?

@Netail please check open-telemetry/opentelemetry-js#4572 for more info on the update plan, thanks :)

Netail · 2024-05-22T19:42:30Z

on every page render

Every page render is re-resolving every dependency? Or is that when running in next dev Dev-mode that might be re-compiling each time to get recent changes?

Not completely sure, I believe it runs the instrumentation.ts on every initial page render in development (local) mode.

legendecas · 2024-05-24T13:00:12Z

systeminformation doesn't want to solve this issue at their side: sebhildebrandt/systeminformation#230, and the module 'osx-temperature-sensor' can be externalized to avoid being bundled without any plugin configurations.

// webpack.config.js
module.exports = {
  externals: ['osx-temperature-sensor'],
};

I'd avoid using subpath imports which are not considered as public interface.

Additionally, I would suggest splitting the PR for distinct purposes.

trentm · 2024-05-24T23:00:12Z

We discussed in the SIG a couple days ago (https://docs.google.com/document/d/1tCyoQK49WVcE-x8oryZOTTToFm7sIeUhxFPm9g-qL1k/edit#heading=h.bxgyx12a4wsc) and decided that if we pinned the systeminformation dep, we'd be okay using the deep import.

From memory, my summary of the justification is:

A macOS user of @opentelemetry/host-metrics and Webpack may not be able to easily understand the impact of the error/warning message and know how to cope with it.
We could document the work-around in the host-metrics README or perhaps more visible "Troubleshooting" docs on opentelemetry.io, I suppose.
The use of host-metrics could possibly be indirect from the user's p.o.v. if, for example, the package is included in a custom vendor SDK.
The use of webpack could be indirect from the user's point of view. E.g. for Next.js users.
Certainly attributing the error message about osx-temperature-sensor to something used by host-metrics is indirect.
The structure of systeminformation looks like a top-level index that re-exports a number of independent functional sub-packages. I'm guessing (this is my opinion here) that it is unlikely that a future version would break the deep import we are proposing to use. At a guess, the most likely thing that would break this is if systeminformation/package.json added an "exports" section for unrelated reasons.
Pinning and using a deep import kind of sucks, yes.
If was mentioned that considering pulling in just the functionality we want from systeminformation into this repo was undesired. Having an effective fork means we lose the connection for possible fixes. There could be licensing issues/pain.

Aside: I'm not sure if bundlers other than Webpack result in a similar error/warning.

TODO:

Pin the systeminformation version in package.json.
Document some of the above concerns in the code where the deep import is being made to help future maintenance.
Splitting into two PRs, as legendecas suggested, would really help clarify for future maintenance, I agree.

(@Netail I see you recently commited removing the deep import. Sorry for back and forth on this. I should have posted notes from the SIG a couple days ago.)

Netail · 2024-05-24T23:31:44Z

Alright! Thanks, sounds good, will pick it up :) I've created a seperate PR for the semconv alignment & documentation; #2240

codecov · 2024-05-30T16:40:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.40%. Comparing base (dfb2dff) to head (79555fc).
Report is 148 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2071      +/-   ##
==========================================
- Coverage   90.97%   90.40%   -0.58%     
==========================================
  Files         146      149       +3     
  Lines        7492     7514      +22     
  Branches     1502     1573      +71     
==========================================
- Hits         6816     6793      -23     
- Misses        676      721      +45

Files	Coverage Δ
...ages/opentelemetry-host-metrics/src/BaseMetrics.ts	`100.00% <100.00%> (ø)`
packages/opentelemetry-host-metrics/src/metric.ts	`100.00% <100.00%> (ø)`
...ges/opentelemetry-host-metrics/src/stats/common.ts	`97.91% <100.00%> (ø)`
...ackages/opentelemetry-host-metrics/src/stats/si.ts	`66.66% <100.00%> (ø)`

... and 52 files with indirect coverage changes

Netail · 2024-05-30T16:40:06Z

Alright, I've put the deep import back & the semconv alignment PR has been merged in a seperate PR. :)

legendecas

LGTM % nits.

packages/opentelemetry-host-metrics/package.json

packages/opentelemetry-host-metrics/tsconfig.json

legendecas · 2024-05-30T16:52:23Z

packages/opentelemetry-host-metrics/test/metric.test.ts

@@ -25,7 +26,7 @@ import {
 MetricReader,
 } from '@opentelemetry/sdk-metrics';
 import * as assert from 'assert';
-import * as os from 'os';
+import * as os from 'node:os';


I'd avoid mixing the style of with and without the prefix node:. Could you remove unrelated changes?

I will remove them (noticed I forgot to prefix assert), but we do need to add them for the future though incase people use different js runtimes e.g. bun or deno.

And to bypass require cache.

import-js/eslint-plugin-import#2717 (comment)

https://stateful.com/blog/node-18-prefix-only-modules

If, for example, we needed to use node:test -- which is only available with the node:-prefix -- then yes we'd need to use the prefix.

However, currently we support "engines": { "node": ">=14" } in most package.json files. That is relatively loose on defining which Node.js v14 versions we support, but at least v14.0.0 didn't support node: prefixes:

Welcome to Node.js v14.0.0. Type ".help" for more information. > require('node:os') Uncaught Error: Cannot find module 'node:os' Require stack: - <repl> at Function.Module._resolveFilename (internal/modules/cjs/loader.js:1014:15) at Function.Module._load (internal/modules/cjs/loader.js:884:27) at Module.require (internal/modules/cjs/loader.js:1074:19) at require (internal/modules/cjs/helpers.js:72:18) { code: 'MODULE_NOT_FOUND', requireStack: [ '<repl>' ] } >

Also, there is no rush on the usage with different runtimes. My guess (granted a guess), is that there is a ton of other work that would be required to get many of the OTel packages working reasonably with bun, deno, etc. We'd want to coordinate that in separate PRs.

Oh that's true, do note that 14 and 16 hit their eol. So should we still support it tho?

trentm

Thanks for persisting.

packages/opentelemetry-host-metrics/src/BaseMetrics.ts

packages/opentelemetry-host-metrics/src/stats/si.ts

Netail · 2024-06-03T09:47:52Z

fyi; I am not allowed to merge, so when you guys are ready. feel free to merge :)

legendecas · 2024-06-03T10:45:54Z

Thank you for working on this!

fix: semcov alignment & reduce import bundle

2c6d8cf

Netail requested a review from a team as a code owner March 30, 2024 00:00

github-actions bot assigned legendecas Mar 30, 2024

github-actions bot requested a review from legendecas March 30, 2024 00:00

github-actions bot added the pkg:host-metrics label Mar 30, 2024

pichlermarc reviewed Apr 2, 2024

View reviewed changes

packages/opentelemetry-host-metrics/src/stats/si.ts Show resolved Hide resolved

Netail added 3 commits April 2, 2024 16:08

fix: types?

60e0933

Merge branch 'main' into fix/host-metrics-bundling

65aabe7

fix: ci/cd

ae578fc

Merge branch 'main' into fix/host-metrics-bundling

ce92ffe

Netail requested a review from pichlermarc April 2, 2024 21:28

Netail added 2 commits April 3, 2024 15:51

Merge branch 'main' into fix/host-metrics-bundling

e818ba5

Merge branch 'main' into fix/host-metrics-bundling

2484ef6

docs: specify semantic conventions

1106e97

david-luna reviewed Apr 5, 2024

View reviewed changes

Netail force-pushed the fix/host-metrics-bundling branch from 0b09d19 to f28832f Compare April 5, 2024 11:55

revert some changes

8f65520

Netail force-pushed the fix/host-metrics-bundling branch from f28832f to 8f65520 Compare April 5, 2024 11:58

Merge branch 'main' into fix/host-metrics-bundling

08506a4

Netail added 2 commits May 22, 2024 21:42

Merge branch 'main' into fix/host-metrics-bundling

06ac5e7

Merge branch 'main' into fix/host-metrics-bundling

a3f9a61

Merge branch 'main' into fix/host-metrics-bundling

fc23083

Netail changed the title ~~fix(host-metrics): semcov alignment & macOS fix~~ fix(host-metrics): semcov alignment May 24, 2024

fix: remove subpath

f675811

Netail force-pushed the fix/host-metrics-bundling branch from 591bb0a to f675811 Compare May 24, 2024 13:20

Netail changed the title ~~fix(host-metrics): semcov alignment~~ fix(host-metrics): bundling fix May 24, 2024

Netail changed the title ~~fix(host-metrics): bundling fix~~ fix(host-metrics): macOS bundling fix May 24, 2024

Netail added 3 commits May 30, 2024 18:12

Merge branch 'main' into fix/host-metrics-bundling

9c14d64

feedback

72ba6f9

lint

4c0c3c9

legendecas reviewed May 30, 2024

View reviewed changes

Netail added 4 commits May 30, 2024 22:20

Merge branch 'main' into fix/host-metrics-bundling

f0a7187

remove node import prefix

c0fbc8b

lint

5338456

eof

3df9163

legendecas approved these changes May 31, 2024

View reviewed changes

Merge branch 'main' into fix/host-metrics-bundling

79555fc

trentm approved these changes May 31, 2024

View reviewed changes

packages/opentelemetry-host-metrics/src/BaseMetrics.ts Show resolved Hide resolved

packages/opentelemetry-host-metrics/src/stats/si.ts Show resolved Hide resolved

legendecas merged commit 7d6ddea into open-telemetry:main Jun 3, 2024
19 checks passed

dyladan mentioned this pull request Jun 3, 2024

chore: release main #2211

Merged

Netail deleted the fix/host-metrics-bundling branch June 3, 2024 11:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(host-metrics): macOS bundling fix #2071

fix(host-metrics): macOS bundling fix #2071

Netail commented Mar 30, 2024 •

edited

linux-foundation-easycla bot commented Mar 30, 2024 •

edited

Netail commented Apr 2, 2024 •

edited

david-luna commented Apr 4, 2024

Netail commented Apr 4, 2024 •

edited

Netail commented Apr 4, 2024 •

edited

david-luna commented Apr 4, 2024

Netail commented Apr 4, 2024 •

edited

Netail commented Apr 4, 2024

david-luna commented Apr 4, 2024

Netail commented Apr 4, 2024 •

edited

david-luna Apr 5, 2024

Netail Apr 5, 2024 •

edited

david-luna commented Apr 5, 2024

Netail commented May 22, 2024

legendecas commented May 24, 2024 •

edited

trentm commented May 24, 2024

Netail commented May 24, 2024 •

edited

codecov bot commented May 30, 2024 •

edited

Netail commented May 30, 2024

legendecas left a comment

legendecas May 30, 2024

Netail May 30, 2024 •

edited

trentm May 30, 2024

Netail May 30, 2024

trentm left a comment

Netail commented Jun 3, 2024

legendecas commented Jun 3, 2024

fix(host-metrics): macOS bundling fix #2071

fix(host-metrics): macOS bundling fix #2071

Conversation

Netail commented Mar 30, 2024 • edited

Which problem is this PR solving?

Short description of the changes

linux-foundation-easycla bot commented Mar 30, 2024 • edited

Netail commented Apr 2, 2024 • edited

david-luna commented Apr 4, 2024

Netail commented Apr 4, 2024 • edited

Netail commented Apr 4, 2024 • edited

david-luna commented Apr 4, 2024

Netail commented Apr 4, 2024 • edited

Netail commented Apr 4, 2024

david-luna commented Apr 4, 2024

Netail commented Apr 4, 2024 • edited

david-luna Apr 5, 2024

Choose a reason for hiding this comment

Netail Apr 5, 2024 • edited

Choose a reason for hiding this comment

david-luna commented Apr 5, 2024

Netail commented May 22, 2024

legendecas commented May 24, 2024 • edited

trentm commented May 24, 2024

Netail commented May 24, 2024 • edited

codecov bot commented May 30, 2024 • edited

Codecov Report

Netail commented May 30, 2024

legendecas left a comment

Choose a reason for hiding this comment

legendecas May 30, 2024

Choose a reason for hiding this comment

Netail May 30, 2024 • edited

Choose a reason for hiding this comment

trentm May 30, 2024

Choose a reason for hiding this comment

Netail May 30, 2024

Choose a reason for hiding this comment

trentm left a comment

Choose a reason for hiding this comment

Netail commented Jun 3, 2024

legendecas commented Jun 3, 2024

Netail commented Mar 30, 2024 •

edited

linux-foundation-easycla bot commented Mar 30, 2024 •

edited

Netail commented Apr 2, 2024 •

edited

Netail commented Apr 4, 2024 •

edited

Netail commented Apr 4, 2024 •

edited

Netail commented Apr 4, 2024 •

edited

Netail commented Apr 4, 2024 •

edited

Netail Apr 5, 2024 •

edited

legendecas commented May 24, 2024 •

edited

Netail commented May 24, 2024 •

edited

codecov bot commented May 30, 2024 •

edited

Netail May 30, 2024 •

edited