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

Added os.description resource attribute #42

Merged

Conversation

surbhiia
Copy link
Contributor

@surbhiia surbhiia commented Aug 23, 2023

As per our discussion in previous 2 client sig meetings, adding os.description attribute which would contain build number and API level as well. Below screenshot shows what os.description string looks like from debug log.

Screenshot 2023-08-29 at 1 11 15 PM

Will soon create the other PR - proposing a change in semantic conventions to add os.build and os.api.level attribute. Thanks!

@surbhiia surbhiia requested a review from a team August 23, 2023 20:23
@surbhiia surbhiia marked this pull request as draft August 23, 2023 20:41
@surbhiia surbhiia marked this pull request as ready for review August 23, 2023 21:20
@LikeTheSalad
Copy link
Contributor

Thanks for your PR @surbhiia!

I didn't join this week's client sig meeting, so I'm not fully aware of what's the consensus on what should be shown in the os.description resource attr, and I'm also not sure if it's still up for debate, though I wanted to mention that from a practical point of view I think the API level could be very valuable when identifying an OS version, since in my experience, it along with Build.MANUFACTURER, Build.MODEL and Build.VERSION.RELEASE are enough to narrow down the environment conditions of an android app. The API level isn't necessarily a 1:1 relation to the version, as shown here with the android version 12 for example, so I find it handy to have when it comes to finding a root cause of an issue.

That being said, if this decision was already made as final in the previous meeting, then I'm ok to move forward with it. However, if that's not the case yet, then I'd like to discuss it a bit further in the next client meeting if you don't mind!

@surbhiia surbhiia force-pushed the develop/add-resource-attributes branch from 0c6cc6e to 4966b1a Compare August 24, 2023 18:14
@surbhiia surbhiia force-pushed the develop/add-resource-attributes branch from 4966b1a to 3ad5c83 Compare August 24, 2023 18:21
@breedx-splk
Copy link
Contributor

The API level isn't necessarily a 1:1 relation

That's right, which I think is why the build number is being included here in the description. You should be able to get the API level from the build string, right?

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

Looks good to me, just need to change the local variable name to match idiomatic standards. Thanks!

@surbhiia
Copy link
Contributor Author

surbhiia commented Aug 24, 2023

Thanks @breedx-splk and @LikeTheSalad for looking into it.

We can identify the API level from the version number itself as shown here.

We can definitely discuss it more if needed, in next week’s client sig meeting. In the last meeting we decided the following:

  • Add os.description in opentelementry - android repo as build number is missing today from emitted telemetry and this field can contain that. iOS today does the same and we kept the string syntax same as iOS.
  • Propose a new os.build attribute to be added in semantic-conventions that can include the build number and we won’t need to populate os.description field for it. Today the semantic conventions mention that the version field should contain the entire string that helps identify the build artifact. But as different platforms have different syntax for build numbers, we decided to add another os.build field to capture that instead of having to use os.version for it. Everybody did mention that it might be difficult to get it added as those conventions are in stable state.

My rationale behind having os.build as well is following - I think we should be able to identify the exact OS code change from the telemetry data for the cases when we need it for troubleshooting, not having it there would mean we are missing a piece of information. While google won’t introduce major api changes in subsequent builds under same version, it can introduce bug fixes, performance enhancements or security patches which could affect how an application interacts with the OS or how permissions are handled etc which could affect an apps performance or introduce bugs etc. So, it is important to have complete OS information in telemetry. I liked the idea of having a separate os.build attribute but as it will take some time to get it added, we can go with os.description for it for now.

@LikeTheSalad
Copy link
Contributor

LikeTheSalad commented Aug 25, 2023

We can identify the API level from the version number itself as shown here.

I tried to look for the ID in the image (OSR1.180418.028.A1) with no luck unfortunately. This is my concern with this information since the Android OS is so fragmented across so many manufacturers, some even not officially recognized by Google, such as Huawei for example, I'm not sure if we'd always be able to get an ID that's tracked by Google in this list which seems to cover only nexus and pixels. I even tried to google "OSR1.180418.028.A1" with no luck either.

I understand and I agree with the fact that we need to know the exact build environment for proper troubleshooting 👍 and I know that using the Build.ID for this sounds sensible and it seems like it works for iOS as well maybe because the manufacturer is only one, but in a practical sense, I'm afraid that I'm not sure how to use this value to figure out details that would help me during a troubleshooting. In my experience, the combination of the os version, manufacturer/model and API level take me straight to the device I need to focus on to fix an issue. There are probably some edge cases where more detailed information such as the build ID might be helpful, so if we end up adding a new os.build attr I think it would be fine to have more data in case it's needed, though talking from my experience, version, manufacturer/model, and API level are the coordinates to find and fix pretty much any issue.

@surbhiia
Copy link
Contributor Author

"OSR1.180418.028.A1" might not be searchable because it is from an android emulator. But I agree that there might be no mapping docs like Google's for other manufacturer builds and the point that build Id might be relevant for only certain edge cases where it would be helpful for either determining the exact changes that went in (in cases where that is available) or just identifying that a particular build number shows up in all failures... so either that can be worked out with the manufacturer ( like reporting a possible bug in an OS build) or changes could be figured out if need be with build number as a starting point.

I agree it might not be useful in most cases. We can leave it up to the app devs who know their application needs better to decide if they would like to add it. We can provide apis like setGlobalAttributes to support that.

Thanks for the detailed discussion on this! :)

@surbhiia
Copy link
Contributor Author

Added API level to os.description string as discussed in today's client sig meeting!

Copy link
Contributor

@LikeTheSalad LikeTheSalad left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for your patience @surbhiia!

@LikeTheSalad LikeTheSalad merged commit a8f7e6d into open-telemetry:main Aug 30, 2023
1 check passed
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.

3 participants