Skip to content

Mobile spec additions #1759

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

Merged
merged 17 commits into from
Jul 12, 2021
Merged

Conversation

bryce-b
Copy link
Member

@bryce-b bryce-b commented Jun 14, 2021

Follow up of #1669
I updated the yaml files & verified the various make checks pass.

Changes

  • adds net.host.carrier.*
  • adds net.host.connection_type

Related issues #

#1647 Add semantic convention for describing network connectivity conditions

@bryce-b bryce-b requested review from a team June 14, 2021 17:50
@bryce-b
Copy link
Member Author

bryce-b commented Jun 14, 2021

hm, it seems that make markdownlint and make table-check are at odds.

@bryce-b
Copy link
Member Author

bryce-b commented Jun 14, 2021

I need some assistance resolving this error in the semantic-conventions check. It oddly seems to resolve if a space is arbitrarily added in 7e7a188, but that breaks markdownlint. Is this actually an issue between the yaml spec & the markdown file? How can I view what the tool is generating from the yaml?

@arminru arminru added area:semantic-conventions Related to semantic conventions enhancement New feature or request spec:trace Related to the specification/trace directory labels Jun 15, 2021
@arminru
Copy link
Member

arminru commented Jun 15, 2021

Hi @bryce-b!

I need some assistance resolving this error in the semantic-conventions check. It oddly seems to resolve if a space is arbitrarily added in 7e7a188, but that breaks markdownlint. Is this actually an issue between the yaml spec & the markdown file? How can I view what the tool is generating from the yaml?

The tools' output is done right in-place if you run it as advised in the README, so you should see it right away and commit it together with the YAML change. You'll also have to re-run the tool now anyway since ba64493 introduced wording that is actually a bit different from what the Markdown generator would add, then it should pass again.

@bryce-b
Copy link
Member Author

bryce-b commented Jun 15, 2021

I've already made this comment, but it's in an outdated review comment chain, so I'm re-adding it for visibility.

I deleted all the table markdown I manually added, and let the table-generation add them. (7af45c0) It looks much better, but it seems like the table-generation is adding an additional space at line 64 that is making markdownlint unhappy.

./specification/trace/semantic_conventions/span-general.md:64 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]

@Oberon00
Copy link
Member

Oberon00 commented Jun 15, 2021

Unfortunately, this looks like a bug in the semantic convention generator, when more than one enum is added and there is a (foot)note in one. 😞

I created open-telemetry/build-tools#47

@bryce-b
Copy link
Member Author

bryce-b commented Jun 15, 2021

@Oberon00 phew! I'm glad it wasn't me, haha

@maxgolov
Copy link
Contributor

maxgolov commented Jun 16, 2021

I think, as a minimum, the concepts that need to be gathered are:

  • network cost
  • device id
  • manufacturer: make, model
  • OS arch type
  • possibly a vector of device features: hasKeyboard, hasMouse, hasLTE or WiFi only, hasPhone-alike characteristics, e.g. Dialer which may be a feature even on device that has WiFi only.
  • many (most) devices in developing nations are dual-SIM ; you have to account for the fact that network is a vector, not a single value
  • for any field added - there has to be a separate column, whether this field potentially needs a privacy review. For all of these privacy-sensitive fields - these should be definitely all marked as optional. But mere fact of collecting provider name and network id, deserves a separate asterisk / notion of a potential field that needs a privacy review.

@jkwatson
Copy link
Contributor

I think, as a minimum, the concepts that need to be gathered are:

  • network cost
  • device id
  • manufacturer: make, model
  • OS arch type
  • possibly a vector of device features: hasKeyboard, hasMouse, hasLTE or WiFi only, hasPhone-alike characteristics, e.g. Dialer which may be a feature even on device that has WiFi only.
  • many (most) devices in developing nations are dual-SIM ; you have to account for the fact that network is a vector, not a single value
  • for any field added - there has to be a separate column, whether this field potentially needs a privacy review. For all of these privacy-sensitive fields - these should be definitely all marked as optional. But mere fact of collecting provider name and network id, deserves a separate asterisk / notion of a potential field that needs a privacy review.

I think it's ok to start simple and just pick one thing to specify, rather than trying to boil the ocean and specify every possible thing in a single PR. Let's keep this PR focussed on just describing the network that a mobile device is using for data communications, if that's ok.

@bryce-b bryce-b requested review from jkwatson and arminru June 22, 2021 17:11
@jmacd
Copy link
Contributor

jmacd commented Jun 22, 2021

@maxgolov I read your concerns above and want to acknowledge them. I believe I am correct in saying that most of the issues you raised are about instrumentation, not semantic conventions. The users of these semantic conventions will have to pay attention to privacy, for example. I am under the impression that there is a new SIG that will form for client-side instrumentation, which at this time exists as a discussion in the #otel-client-side-telemetry room of the CNCF slack.

@jmacd
Copy link
Contributor

jmacd commented Jun 30, 2021

This has only one approval since net.host.connection.subtype changed. @open-telemetry/specs-approvers please take another look.

@github-actions
Copy link

github-actions bot commented Jul 8, 2021

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 Jul 8, 2021
@bryce-b bryce-b requested a review from carlosalberto July 8, 2021 14:24
Copy link
Member

@nachoBonafonte nachoBonafonte left a comment

Choose a reason for hiding this comment

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

This is a needed PR for mobile tracking

@bryce-b
Copy link
Member Author

bryce-b commented Jul 9, 2021

@carlosalberto could you re-approve, since there were a few changes since your original approval?

@bryce-b
Copy link
Member Author

bryce-b commented Jul 9, 2021

@jmacd how are we looking for merge?

@github-actions github-actions bot removed the Stale label Jul 10, 2021
@jmacd jmacd merged commit 52cc128 into open-telemetry:main Jul 12, 2021
@jmacd
Copy link
Contributor

jmacd commented Jul 12, 2021

Thanks @bryce-b!

carlosalberto added a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
* added spec changes per open-telemetry#1647

* added 'wired' as possible value to `net.host.connection_type`

* fixed spacing in CHANGELOG.md

* added mobile carrier attributes to  semantic_conventions/trace/general.yaml

* fixed issue revealed by

* reverted extra space per markdownlint

* made connection_type allow custom values

* added additional space

* removed manually added table & ran generator tool

* separated connection.type & connection.subtype

* updated changelog to reflect iteration

* added literal radio tech to connection.subtype table

* added  as option for connect.type

Co-authored-by: Armin Ruech <[email protected]>
Co-authored-by: Carlos Alberto Cortez <[email protected]>
schmikei pushed a commit to schmikei/opentelemetry-specification that referenced this pull request Apr 17, 2025
* added spec changes per open-telemetry#1647

* added 'wired' as possible value to `net.host.connection_type`

* fixed spacing in CHANGELOG.md

* added mobile carrier attributes to  semantic_conventions/trace/general.yaml

* fixed issue revealed by

* reverted extra space per markdownlint

* made connection_type allow custom values

* added additional space

* removed manually added table & ran generator tool

* separated connection.type & connection.subtype

* updated changelog to reflect iteration

* added literal radio tech to connection.subtype table

* added  as option for connect.type

Co-authored-by: Armin Ruech <[email protected]>
Co-authored-by: Carlos Alberto Cortez <[email protected]>
schmikei pushed a commit to schmikei/opentelemetry-specification that referenced this pull request Apr 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions enhancement New feature or request spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants