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

Create app.installation.id attribute #1897

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

bencehornak
Copy link

@bencehornak bencehornak commented Feb 10, 2025

Fixes #1874

Warning

This PR must be merged together with #1951 as they are highly related, dear maintainers, please add them to the same milestone.

Changes

Added app.installation.id (the first attribute in the installation namespace), which is a unique identifier representing the installation of an application on a specific device. Its definition is more-or-less identical to the old definition of device.id, which has always identified app installations and not devices.

In #1951 I am changing the definition of device.id, so that it actually identifies the device.

For more context see #1874.

Merge requirement checklist

@bencehornak bencehornak requested review from a team as code owners February 10, 2025 11:26
@github-actions github-actions bot added the enhancement New feature or request label Feb 10, 2025
Copy link
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

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

I think this changes makes sense given the discussion in the issue. But I think it would be better if the changes related to device would be in a separate PR. Then you can include the changelog for it as a breaking change, since it is now Opt-In.

Copy link

@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.

Thanks! I like the idea of using a name that better describes what we were doing with device.id. I've just added some questions on specific details, but I'm on board with the change.

@bencehornak
Copy link
Author

I think this changes makes sense given the discussion in the issue. But I think it would be better if the changes related to device would be in a separate PR. Then you can include the changelog for it as a breaking change, since it is now Opt-In.

@joaopgrassi Thanks for your feedback! My reasoning for adding two changes to a single PR was that I see the two changes as a single transaction. If, say, the device.id changes are merged first, then the SDK users won't have any recommended fields to identify different installations until the 2nd PR with the installation.id changes are merged. If we do it other way around, for a short period of time there will be significant overlaps between the definitions of device.id and installation.id, so that would also not be ideal.

So right now I don't know, how to put the two changes in topological order given their circular dependency, but if you help me figure this out, I'm happy to split this PR into two.

@joaopgrassi
Copy link
Member

joaopgrassi commented Feb 11, 2025

I think this changes makes sense given the discussion in the issue. But I think it would be better if the changes related to device would be in a separate PR. Then you can include the changelog for it as a breaking change, since it is now Opt-In.

@joaopgrassi Thanks for your feedback! My reasoning for adding two changes to a single PR was that I see the two changes as a single transaction. If, say, the device.id changes are merged first, then the SDK users won't have any recommended fields to identify different installations until the 2nd PR with the installation.id changes are merged. If we do it other way around, for a short period of time there will be significant overlaps between the definitions of device.id and installation.id, so that would also not be ideal.

So right now I don't know, how to put the two changes in topological order given their circular dependency, but if you help me figure this out, I'm happy to split this PR into two.

Yes, I understand. We have been asked by users though that such bundling breaking changes with other changes such as this makes it harder for them to track breaking changes, which is also understandable. so, we have been trying recently to better identify/label breaking changes PRs to help in such cases.

I think we could still make it work to merge both of them separately. You will need approvals in both anyway and once both are good we just merge them together in the same release. We can for example, add the PRs into the same milestone.

@bencehornak
Copy link
Author

@joaopgrassi alright, I'll restrict this PR to the new definition of device.id, and will add the new attribute to a separate PR.

@bencehornak bencehornak changed the title Create installation.id, redefine device.id Create app.installation.id attribute Feb 27, 2025
@bencehornak
Copy link
Author

Sorry folks for the delay, now I'm back from holiday!

@joaopgrassi I have extracted the changes to device.id to #1951 as you requested, this PR only contains the addition of the app.installation.id attribute.

@lmolkova, @LikeTheSalad, @bidetofevil I believe that I have addressed all of your remarks, I would appreciate your feedback on the new additions.

Copy link

@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.

Thank you! 🙏

@bencehornak
Copy link
Author

@joaopgrassi @lmolkova, the two PRs (this and #1951) are ready for your review, could you take a look, if you get a chance?

@trask
Copy link
Member

trask commented Mar 12, 2025

cc @open-telemetry/semconv-mobile-approvers

@trask
Copy link
Member

trask commented Mar 18, 2025

it would be great if we can get approvals from the iOS and browser perspectives as well (if this attribute is applicable there)


On Android this value SHOULD be equal to either:

- [Firebase Installation ID](https://firebase.google.com/docs/projects/manage-installations).

Choose a reason for hiding this comment

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

Any reason why Firebase is called out specifically? Typically, each SDK or library who want to uniquely identify app instances will have their own unique ID. Having the same value for different SDKs for the same app might cause confusion in the recorded telemetry as to their provenance.

Copy link
Author

Choose a reason for hiding this comment

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

I wanted to stay as close to the official recommendation as possible, that's why I called out Firebase and the GUID.

Copy link
Author

Choose a reason for hiding this comment

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

Regarding the ID's provenance, I had this idea to prefix the value with exactly that (e.g. firebase: or guid:), but we later discarded it, see this thread. Feel free to reopen, if you would like to add further points.

Choose a reason for hiding this comment

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

Firebase is not universally used, and using it ties the instrumentation to it, which means tying itself to Firebase. I think it's not an unreasonable recommendation if you use Firebase, but the recommendation in the docs probably makes the most sense to me:

val uniqueID = UUID.randomUUID().toString()

Choose a reason for hiding this comment

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

re: prefix - Ah, I see you were thinking along those lines too, and just using app signals the intention to have skip over that implementation detail. Yeah, makes ense.

Copy link
Author

Choose a reason for hiding this comment

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

Firebase is not universally used, and using it ties the instrumentation to it, which means tying itself to Firebase. I think it's not an unreasonable recommendation if you use Firebase, but the recommendation in the docs probably makes the most sense to me

Couldn't agree more, I'd also not choose to use Firebase in the OTel Android SDK as the primary implementation, because that would pull in a huge dependency, which is unwanted in many cases. I could imagine it being an alternative implementation like this:

OpenTelemetryRum.builder(this, config)
  .appInstallationIdProvider(UuidInstallationIdProvider()) // default behavior
  // or
  .appInstallationIdProvider(FirebaseInstallationIdProvider())
  // ...

where FirebaseInstallationIdProvider could be defined in a separate Firebase-specific Gradle module.

I would resolve this thread, unless you would like to change something in the wording.

Choose a reason for hiding this comment

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

In terms of wording, I think saying it SHOULD be unique, and give some examples of how to get one, e.g. the Fireebase impl, random UUID, etc.. When I first read this, the fact it said "SHOULD be equal to either" seems like a recommendation of sorts. If you can remove that implication, that would be great. If not, it's not a big deal either and wouldn't be blocking to me.

|---|---|---|---|---|
| <a id="app-installation-id" href="#app-installation-id">`app.installation.id`</a> | string | A unique identifier representing the installation of an application on a specific device [1] | `2ab2916d-a51f-4ac8-80ee-45ac31a28092` | ![Development](https://img.shields.io/badge/-development-blue) |

**[1] `app.installation.id`:** Its value SHOULD remain the same between launches of the same installation of an application.

Choose a reason for hiding this comment

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

This should also persist between normal app upgrades so that version differences on the same install can map back to the same ID

Copy link
Author

Choose a reason for hiding this comment

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

Will change the sentence to: Its value SHOULD persist across launches of the same application installation, including through application upgrades.


| Attribute | Type | Description | Examples | Stability |
|---|---|---|---|---|
| <a id="app-installation-id" href="#app-installation-id">`app.installation.id`</a> | string | A unique identifier representing the installation of an application on a specific device [1] | `2ab2916d-a51f-4ac8-80ee-45ac31a28092` | ![Development](https://img.shields.io/badge/-development-blue) |

Choose a reason for hiding this comment

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

Given that there's no easy way to share or manage this value across all the different modules (first and third parties) that comprise an app, the same app instance might emit different values for this depending on instrumentation. How do we want to account for this in the definition - just that a particular app instance can emit different values, or something more formal?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure if I get this, you mean that if the app uses multiple tracking libraries (e.g. Otel Android SDK, Embrace SDK, Crashlytics, Instabug, Sentry, Posthog, ...) then each of these SDKs might have a different ID? Or are you referring to the possibility that an app has multiple instances of an Otel-compatible SDK in it, thus it is described by multiple Otel Resources (with possibly differing app.installation.id attributes)?

Choose a reason for hiding this comment

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

are you referring to the possibility that an app has multiple instances of an Otel-compatible SDK in it

Yeah. This may be me being overly pedantic, but my thinking is that this will likely be set as a resource attribute before the OTel SDK instance starts, so it's up to whoever starts it to populate it with a value.

If your app has one OTel SDK instance running, no problem. There is only one. But if there are multiple OTel SDK instances running at the same time (which is theoretically possible depending on language/impl), we could have different, unrelated code (even third party) setting its own app.installation.id at startup.

A contrived example is if some ad SDK uses OTel under the hood, and it'll start its own specialized OTel SDK instance. This will live along side the instance the app uses for its custom telemetry and whatever other instrumentation you've configured it to use.

Perhaps a warning that this is tied to each OTel SDK instance that's running, and mentioning the multi-instance case as a example of there being more than one, is sufficient?

Copy link
Author

Choose a reason for hiding this comment

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

I see, I think I get what you mean, the possibility to have multiple Otel SDKs in an app is tricky. Ultimately, we need to decide, whether the proposed attribute should identify the 1. app installation or 2. the OTel SDK instance.

Based on the suggested app.installation.id name I would go with the first choice, in which case it would be the user's responsibility to ensure consistency across all OTel SDKs within an app. Practically speaking, I think that should be possible with most (all?) SDKs, at least the OTel SDKs I've worked with allow setting/overriding any resource attributes, including the Android SDK, but that you surely know better than me 😄

If it is intended to have different IDs for each OTel SDK within the app, I suggest to draft a new PR with a new attribute name and optionally reject this one.

I think both IDs make sense in the end, 2. is easier to implement, however 1. is arguably more useful, as it allows correlating the telemetry stemming from each of those SDKs. Perhaps we can agree on the following phrasing:

If multiple OpenTelemetry SDKs are used within the same application, they SHOULD use the same value for app.installation.id

This allows deviation from this, but states that the harmonized app.installation.id is the expected behavior. What do you think?

Choose a reason for hiding this comment

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

That sounds good. I agree that semantically, it's closer to 2, but making it 1 makes more sense in practice. That additional line you added is sufficient for me to close/acknowledge that loophole.

And agreed about it not being best practice to have multiple SDK instances for a single app process. If you're using an Agent-like package like opentelemetry-android or the Embrace Android SDK that offers a gateway to the OTel API, the OTel instance should come from that. Anything else is effectively instrumentation, and they shouldn't instantiate their own SDK instance. And if they do, they better let the app set things like resource attributes :-)

@bryce-b bryce-b self-requested a review March 20, 2025 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Needs More Approval
Development

Successfully merging this pull request may close these issues.

device.id vs installation.id on Android and iOS
9 participants