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

Browser detector module #3292

Merged
merged 20 commits into from Nov 2, 2022
Merged

Conversation

Abinet18
Copy link
Contributor

@Abinet18 Abinet18 commented Oct 4, 2022

Which problem is this PR solving?

Adds a browser detector module to get browser identification attributes like platform, brands, mobile, language and add it to the resource attributes of the traces. Improvement of the same purpose PR(#3201) that modifies the detector in @opentelemetry/resources. Since the resources was a stable package , we agreed on creating a new experimental package for the browser detector.

Short description of the changes

Added a new experimental package @opentelemetry/opentelemetry-browser-detector , added BrowserDetector module that queries for navigator.userAgentData if supported or just navigator.userAgent and returns the resulting resource object

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

  • Unit test written
  • importing this module and checking the traces contain the browser attributes in the resource span

Checklist:

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

@Abinet18 Abinet18 requested a review from a team as a code owner October 4, 2022 20:35
@Abinet18
Copy link
Contributor Author

Abinet18 commented Oct 6, 2022

@legendecas , can you look into this when you get some time

@legendecas
Copy link
Member

@Abinet18 Sorry for the delay, I'm on vacation this week. I will take a look at this when I'm back. Thank you for your work on this!

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.

Thank you for working on this! Overall LGTM % a few nits.

@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

Merging #3292 (ef9fbac) into main (a3e40da) will decrease coverage by 0.65%.
The diff coverage is 18.51%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3292      +/-   ##
==========================================
- Coverage   93.52%   92.87%   -0.66%     
==========================================
  Files         244      231      -13     
  Lines        7386     6677     -709     
  Branches     1529     1386     -143     
==========================================
- Hits         6908     6201     -707     
+ Misses        478      476       -2     
Impacted Files Coverage Δ
...ntelemetry-browser-detector/src/BrowserDetector.ts 15.38% <15.38%> (ø)
...ckages/opentelemetry-browser-detector/src/types.ts 100.00% <100.00%> (ø)
api/karma.worker.js 0.00% <0.00%> (-100.00%) ⬇️
packages/opentelemetry-resources/karma.worker.js 0.00% <0.00%> (-100.00%) ⬇️
...kages/opentelemetry-sdk-trace-base/karma.worker.js 0.00% <0.00%> (-100.00%) ⬇️
...lemetry-resources/src/detectors/BrowserDetector.ts 53.33% <0.00%> (-46.67%) ⬇️
...lemetry-resources/src/detectors/ProcessDetector.ts 95.45% <0.00%> (-4.55%) ⬇️
...-sdk-trace-web/src/enums/PerformanceTimingNames.ts
...-instrumentation-fetch/src/enums/AttributeNames.ts
...ation-xml-http-request/src/enums/AttributeNames.ts
... and 13 more

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

@MSNev
Copy link
Contributor

MSNev commented Oct 19, 2022

LGTM, if you can just address the lint and changelog check failures we should be good

@Abinet18
Copy link
Contributor Author

@dyladan Is there something I can do to fix the failing checks ?

@legendecas
Copy link
Member

legendecas commented Oct 28, 2022

@Abinet18 You can run npm run update-ts-references at the root of the project to update TS project references to ensure the compilation order. Also, in order to run the browser tests, https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-resources/package.json#L25 need to be configured.

Or I can help to set up the miscellaneous configurations for the browser testing and releases, if you are ok with it.

@Abinet18
Copy link
Contributor Author

@legendecas , I am ok if you setup the required configs.

@legendecas legendecas force-pushed the Browser_Detector_Module branch 2 times, most recently from 20c3cee to 5fe7796 Compare November 1, 2022 09:26
@legendecas
Copy link
Member

Updated with browser testing and fixed lerna package linking issues.

@legendecas
Copy link
Member

@dyladan @MSNev would you mind taking another look? thank you!

@MSNev
Copy link
Contributor

MSNev commented Nov 1, 2022

@dyladan LGTM

@dyladan dyladan merged commit 2cee814 into open-telemetry:main Nov 2, 2022
@Abinet18 Abinet18 deleted the Browser_Detector_Module branch November 2, 2022 15:58
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.

None yet

5 participants