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

fix(resources): fix browser compatibility for host and os detectors #3004

Merged
merged 2 commits into from
Jun 1, 2022

Conversation

legendecas
Copy link
Member

Which problem is this PR solving?

Fixes #3003

Short description of the changes

  1. HostDetector and OSDetector act as noop detectors in browser environments.
  2. BrowserDetector should work on WebWorkers too (navigator API is available on WorkerGlobalScope).

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Enabled WebWorker tests
  • Browser tests on HostDetector and OSDetector.

Checklist:

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

@legendecas legendecas marked this pull request as ready for review May 30, 2022 16:14
@legendecas legendecas requested a review from a team as a code owner May 30, 2022 16:14
@codecov
Copy link

codecov bot commented May 30, 2022

Codecov Report

Merging #3004 (e3f2e93) into main (fcef5e2) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3004      +/-   ##
==========================================
- Coverage   92.79%   92.78%   -0.01%     
==========================================
  Files         183      185       +2     
  Lines        6137     6144       +7     
  Branches     1301     1300       -1     
==========================================
+ Hits         5695     5701       +6     
- Misses        442      443       +1     
Impacted Files Coverage Δ
...ges/opentelemetry-resources/src/detectors/index.ts 100.00% <ø> (ø)
packages/opentelemetry-resources/karma.worker.js 100.00% <100.00%> (ø)
...lemetry-resources/src/detectors/BrowserDetector.ts 100.00% <100.00%> (ø)
...ntelemetry-resources/src/detectors/NoopDetector.ts 100.00% <100.00%> (ø)
...emetry-resources/src/platform/node/HostDetector.ts 76.47% <100.00%> (ø)
...elemetry-resources/src/platform/node/OSDetector.ts 86.66% <100.00%> (ø)
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️

@blumamir
Copy link
Member

blumamir commented May 30, 2022

Thanks for working on this fix.
Since contrib build is currently broken and this PR is a blocker for the contrib fix, I suggest that we merge it soon and then publish a patch 1.3.1

Relevant PR in contrib:
open-telemetry/opentelemetry-js-contrib#1042

@dyladan
Copy link
Member

dyladan commented May 31, 2022

Can you bump the patch version? When it merges I'll release it.

Copy link
Member

@rauno56 rauno56 left a comment

Choose a reason for hiding this comment

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

👍 Why making HostDetector Noop instead of omitting the export altogether?

@legendecas
Copy link
Member Author

👍 Why making HostDetector Noop instead of omitting the export altogether?

We need to make sure our exported names are consistent in different environments so that people don't get TypeError for missing detectors.

@rauno56
Copy link
Member

rauno56 commented Jun 1, 2022

Is that an issue with compilation tools that produce the same bundle for browser and server environments, because if they're importing OsDetector in browser they're definitely not getting what they intended and TypeError generally is applicable.

@legendecas
Copy link
Member Author

ESM is static by design, we cannot conditionally import or export specific names (with legit syntax). Even if we can achieve conditional exports by different entry points, I believe it would be more intuitive to stick to that principle.

Further, detectors are capable of "detecting" the environmental attributes. If they cannot detect anything they recognize, they should act as a no-op and return an empty resource, like what aws-detector, gcp-detector, alibaba-cloud-detector are doing.

@dyladan dyladan mentioned this pull request Jun 1, 2022
2 tasks
@dyladan dyladan merged commit 7649cf1 into open-telemetry:main Jun 1, 2022
@legendecas legendecas deleted the resource-detector branch June 1, 2022 15:13
@rauno56
Copy link
Member

rauno56 commented Jun 2, 2022

Yeah.. It all makes sense, I'm just surprised that we can switch between node and browser in the builds, but not between node and nothing. I guess I'm missing something fundamental from the build step that's enabling this and/or ESM.

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.

New resource detectors import "os" which doesn't work in browsers
5 participants