Browser identification resource attributes

[Abinet18]

Which problem is this PR solving?

For all the traces that are generated from browser ,we need to have the browser identifier attributes like platform, brands, mobile, and user_agent. Instead of adding these attributes to individual spans, it is better if we have them as part of resource attributes which will be common for all the traces. This PR is adding these attributes at WebTracerProvider level.

Short description of the changes

At WebTracerProvider add these browser identifier attributes to the config and pass it to the trace provider constructor.

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• [ x ] New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [ x ] This change requires a documentation update

How Has This Been Tested?

The change is tested on a sample app that uses the otel instrumentation libraries for browser and confirming that the resource attributes include the ones added here

Checklist:

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Abinet18 (573f0c8, feb6d7c)

[legendecas]

Thank you for contributing to the project!

However, I don't think this is the expected approach to adding browser resources. This should be added to the ResourceDetector implementation, like https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-resources/src/detectors/BrowserDetector.ts.

BTW, you'll need to sign the CLA before we can accept your contribution.

[Abinet18]

Thanks for reviewing. I have signed the CLA. And regarding the BrowserDetector, I don't see it being used anywhere except in the tests. Is it possible to use it in the WebTraceProvider to initialize the browser resource attributes ? what is your thought on how to do this ?

[legendecas]

@Abinet18 we currently didn't automatically detect resources in the web packages. The detector has to be invoked manually to detect browser resource attributes. Ideally, we can create a package @opentelemetry/sdk-web that is analogous to the @opentelemetry/sdk-node that set up common platform attributes.

As the first step, I'd encourage adding support to detect the new resource attributes in the browser resource detector.

[Abinet18]

I have done as you advised, brought the attribute addition to the Browser detector. I also wanted to use SemanticResourceAttributes for the attribute names but these names are not yet included there ?

[codecov]

Codecov Report

Merging #3201 (4ad3c46) into main (32cb123) will decrease coverage by 0.91%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3201      +/-   ##
==========================================
- Coverage   93.28%   92.36%   -0.92%     
==========================================
  Files         201      195       -6     
  Lines        6579     6118     -461     
  Branches     1379     1291      -88     
==========================================
- Hits         6137     5651     -486     
- Misses        442      467      +25     

Impacted Files	Coverage Δ
...lemetry-resources/src/detectors/BrowserDetector.ts	100.00% <100.00%> (ø)
packages/opentelemetry-resources/src/types.ts	100.00% <100.00%> (ø)
.../src/platform/browser/export/BatchSpanProcessor.ts	47.61% <100.00%> (+2.61%)	⬆️
packages/opentelemetry-sdk-trace-web/src/utils.ts	65.62% <0.00%> (-29.38%)	⬇️
...-trace-base/src/platform/node/RandomIdGenerator.ts	87.50% <0.00%> (-6.25%)	⬇️
...emetry-instrumentation-xml-http-request/src/xhr.ts
...s/opentelemetry-instrumentation-fetch/src/fetch.ts
...-instrumentation-fetch/src/enums/AttributeNames.ts
...mentation-xml-http-request/src/enums/EventNames.ts
...ation-xml-http-request/src/enums/AttributeNames.ts
... and 3 more

[legendecas]

Thank you for your work on this! Can you also cover the new feature with a test please?

[legendecas]

Generally LGTM! Just a few nits.

[legendecas]

Regarding the semantic convention attributes, we can create a constant map for it and add a comment that it should be replaced once the semantic convention package is updated. So that we don't need to repeat these constants here and in tests.

[Abinet18]

Check if the change is as you requested. Also we have redundant attributes in version and user_agent. What do you suggest ?

[scheler]

@martinkuba can you review this - the process.runtime.* attributes were here before you added the browser.* attributes in the semantic conventions - do we really need process.runtime.* attributes as part of BrowserDetector?

cc: @MSNev @t2t2

[legendecas]

I believe removing the existing attributes from the detector can be breaking. As the browser.* attributes are the newer ones, is it possible to remove the duplicated entry browser.user_agent?

[scheler]

@martinkuba do we need to include the user_agent attribute even when platform, brands and mobile are present or only when they are not available?

[martinkuba]

The intent was to include it only when the userAgentData API is not available, so that backends move to using the new API. Parsing the userAgent string should only be done to support old browsers.

[Abinet18]

Should I then remove the user_agent attribute. The runtime version attribute is always added and it is the userAgent, so browsers that don't support userAgentData can parse it.

[scheler]

The process.runtime.* attributes should be deprecated in browser resource attributes now that we have browser.* attributes in the spec. However, browser.user_agent should be included only in an else block when userAgentData isn't supported by the browser.

I wonder what the process for this deprecation is. Just add to changelog?

[martinkuba]

The process.runtime.version attribute probably should no longer be used for browsers, so that the user-agent string is not sent twice. But I am not sure that process.runtime.name should be deprecated. All of the browser attributes are optional, so it is possible that they would not be sent if the browser does not provide them. On the other hand, the value for process.runtime.name is the static value browser; it is added by the SDK without detecting anything, which makes it more reliable.

[scheler]

@legendecas Can you respond to my comment above? We could discuss it in the JS SIG too this week.

[legendecas]

Sorry for the late response.

The intention of the specified attributes is to write down what's already shipped.

As you mentioned removing existing attributes can be breaking. Package @opentelemetry/resource is declared stable so I'm not sure about introducing such a change to the existing API is fine. One possible solution is to add a new detector like UserAgentDataDetector, which implements the newer browser.* attributes, and in the meantime, we can mark the BrowserDetector as deprecated.

[Abinet18]

So , Do you mean that we should drop this change and create a new Detector for the user-agent.

[legendecas]

Yes, if we need to avoid reporting the legacy process attributes for the browser, we may add a new detector to not introduce breakage.

[scheler]

We discussed in the JS SIG today 9/14 - it was suggested that we add the new detector in a separate package in the experimental folder so that its release can be managed independently.

Separately, I will submit a PR to the spec repo and get the process.runtime.* attributes removed for the browser case, after we which we can mark the BrowserDetector in this package as deprecated.