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

@opentelemetry/resources use fs/promise,but it's from node version 14. #3673

Closed
stone-jin opened this issue Mar 14, 2023 · 4 comments · Fixed by #3681
Closed

@opentelemetry/resources use fs/promise,but it's from node version 14. #3673

stone-jin opened this issue Mar 14, 2023 · 4 comments · Fixed by #3681
Assignees
Labels
priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization

Comments

@stone-jin
Copy link

What happened?

Steps to Reproduce

Expected Result

Actual Result

Additional Details

this version use fs/promise, from node doc: https://nodejs.org/dist/latest-v18.x/docs/api/fs.html#promises-api, it's from node version 14. so many node app below 14 are effect.

https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-resources/src/platform/node/machine-id/getMachineId-linux.ts

OpenTelemetry Setup Code

No response

package.json

No response

Relevant log output

No response

@stone-jin stone-jin added bug Something isn't working triage labels Mar 14, 2023
@wesleytodd
Copy link

wesleytodd commented Mar 14, 2023

This broke some builds for us. I understand that it (node@12) is on it's way out, but since this project doesn't use the engines field (not saying it should) then this change was breaking and should have been part of a major version bump (feat!: for the conventional commit). Another way would be to enforce that commits like this are always considered breaking changes to land in the next major.

@dyladan
Copy link
Member

dyladan commented Mar 15, 2023

We do have the engines field in that package: https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-resources/package.json#L44

WRT the commit you referenced, we realize that was a breaking change. In the future, we will not drop support for runtimes without a major version bump. We may be able to fix this particular issue for node 12, but it is not a supported version so there is no guarantee and it is not tested.

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

dyladan commented Mar 15, 2023

Removing the bug label since this is not for a supported runtime version

@wesleytodd
Copy link

We do have the engines field in that package

Sorry I didn't actually check that, I made an assumption based on the internal bug report I got. My bad, sorry. I wonder how our user got into this state without noticing all those warnings then. I think they were already in the process of updating the old project, so maybe nothing to follow up there.

WRT the commit you referenced, we realize that was a breaking change. In the future, we will not drop support for runtimes without a major version bump

👍 And totally understand if you don't fix. I just wanted to make sure folks were on the same page on dropping node version support being breaking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization
Projects
None yet
4 participants