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.

Sign up for GitHub

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

Jump to bottom

Add Resource browser attributes #2353

Merged
merged 25 commits into from
May 19, 2022
Merged

Add Resource browser attributes #2353

merged 25 commits into from
May 19, 2022

Conversation

martinkuba
Copy link
Contributor

@martinkuba martinkuba commented Feb 16, 2022 •
edited
Loading

This is the same as #2115, except it only adds browser attributes. It does not include the addition of app attributes as in #2115.

@martinkuba martinkuba requested review from a team February 16, 2022 15:18
@martinkuba martinkuba changed the title Add Resource browser and app attributes, clarify device attributes as client-side only Add Resource browser attributes, clarify device attributes as client-side only Feb 16, 2022
@arminru arminru added area:semantic-conventions Related to semantic conventions spec:resource Related to the specification/resource directory labels Feb 22, 2022
@github-actions
Copy link

github-actions bot commented Mar 3, 2022

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 3, 2022
@arminru arminru removed the Stale label Mar 3, 2022
@arminru arminru requested a review from a team March 3, 2022 12:31
@martinkuba martinkuba force-pushed the client-side-attributes branch from a5e0401 to 14557c0 Compare March 23, 2022 00:27
@martinkuba martinkuba mentioned this pull request Mar 24, 2022
Closed
6 tasks
@scheler scheler mentioned this pull request Mar 25, 2022
Closed
6 tasks
@martinkuba
Copy link
Contributor Author

@MSNev @dyladan I have updated the PR to match the low entropy user-agent client hints. These can be retrieved synchronously, and I think are useful for most common analysis.

@arminru @scheler I have updated the description for the device attributes.

@martinkuba martinkuba mentioned this pull request Apr 1, 2022
Closed
@github-actions
Copy link

github-actions bot commented Apr 5, 2022

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Apr 5, 2022
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Apr 12, 2022
@arminru
Copy link
Member

arminru commented Apr 12, 2022

This is pending on the discussion in #2466.

@arminru arminru reopened this Apr 12, 2022
@martinkuba
Copy link
Contributor Author

@arminru I think these attributes are needed independently of #2466, as we need some way of capturing information about browsers. The attributes are now updated to match user-agent client hints (low-entropy), which cannot be captured in the process.runtime.* attributes alone.

Whether we use presence of these attributes to classify browser telemetry can be a separate discussion. I originally combined those two topics in this PR, but now realize that it may just confuse the matter. I can update the description of the PR and perhaps remove the part about device attributes?

@github-actions github-actions bot removed the Stale label Apr 13, 2022
@arminru
Copy link
Member

arminru commented Apr 15, 2022

@martinkuba

I can update the description of the PR and perhaps remove the part about device attributes?

I think the device part looks good and is small and related enough to be discussed in the same PR but you could also split this up if you prefer, sure.

@martinkuba martinkuba force-pushed the client-side-attributes branch from 0dc9a3e to f0f65a6 Compare May 2, 2022 23:02
dyladan
dyladan approved these changes May 3, 2022
View reviewed changes
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Seems fine to me. Using newer APIs means it will continue being relevant for a longer time than the old APIs even if coverage isn't great right now (missing in at least firefox and safari). The newer APIs are also clearer than the old user agent.

specification/resource/semantic_conventions/browser.md Outdated Show resolved Hide resolved
arminru
arminru approved these changes May 3, 2022
View reviewed changes
Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

LGTM from a general spec/semconv POV. Thank you, @martinkuba!
I'll rely on the Client instrumentation/RUM SIG to asses the suitability of the proposed values and their sources.

specification/resource/semantic_conventions/browser.md Outdated Show resolved Hide resolved
@martinkuba
Copy link
Contributor Author

@yurishkuro This PR needs one more approval before it can be merged. Can you please take a look?

@github-actions github-actions bot removed the Stale label May 6, 2022
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 17, 2022
@arminru arminru requested a review from a team May 17, 2022 15:08
@carlosalberto
Copy link
Contributor

@open-telemetry/specs-approvers Please review before we merge ;)

@github-actions github-actions bot removed the Stale label May 19, 2022
@reyang reyang merged commit 066cbfc into open-telemetry:main May 19, 2022
@scheler scheler mentioned this pull request Aug 30, 2022
Merged
beeme1mr pushed a commit to beeme1mr/opentelemetry-specification that referenced this pull request Aug 31, 2022
@martinkuba @beeme1mr
Add Resource browser attributes (open-telemetry#2353)
3c752cb
beeme1mr pushed a commit to beeme1mr/opentelemetry-specification that referenced this pull request Aug 31, 2022
@martinkuba @beeme1mr
Add Resource browser attributes (open-telemetry#2353)
abcdce6
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@martinkuba
Add Resource browser attributes (open-telemetry#2353)
553a0e2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spencerwilson spencerwilson spencerwilson left review comments

@scheler scheler scheler left review comments

@MSNev MSNev MSNev approved these changes

@arminru arminru arminru approved these changes

@jmacd jmacd jmacd approved these changes

@dyladan dyladan dyladan approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

Assignees

@yurishkuro yurishkuro

Labels
area:semantic-conventions Related to semantic conventions spec:resource Related to the specification/resource directory
Projects
Spec: Client SDK and Instrumentation
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@martinkuba @arminru @carlosalberto @dyladan @scheler @jmacd @spencerwilson @MSNev @yurishkuro @reyang