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

Update the device.app.lifecycle event description and constraints #673

Closed
wants to merge 1 commit into from

Conversation

MSNev
Copy link
Contributor

@MSNev MSNev commented Jan 30, 2024

Changes

Reformat and update the device.app.lifecycle event description adds
constraints for the possible values of the android.state and ios.state.

Merge requirement checklist

@MSNev MSNev requested review from a team January 30, 2024 00:01
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.

I think everything is looking good for Android, I added some comments mostly to clarify some things and to get opinions on the state attr name. But I'm fine to go with these changes as they are. I just would like to wait for @bryce-b's opinion on the iOS part.


<!-- semconv ios.lifecycle.events -->
<!-- semconv device.app.lifecycle(full) -->

Choose a reason for hiding this comment

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

Just curious what does the (full) part means?

example, if the `os.name` attribute is set to `android` then the
`android.state` attribute MUST be used and the `ios.state` attribute MUST NOT
be used. If the `os.name` attribute is set to `ios` then the `ios.state`
attribute MUST be used and the `android.state` attribute MUST NOT be used.

Choose a reason for hiding this comment

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

I know this is the way it was before, although after reading this paragraph, it seems to me that it might not be necessary to prepend the OS name (either android. or ios.) to the state attr name, given that the os.name attr must be present anyway, rendering the OS prefixes in state redundant in my opinion.

Choose a reason for hiding this comment

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

Is the desire to have the prefix due to the fact that we want to allow a separate list of valid values for each?


The `android.state` and `ios.state` attributes are mutually exclusive and MUST

Choose a reason for hiding this comment

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

There are some native mobile apps where there are two platforms at play. For instance, Unity apps are written in a cross-platform way and can be deployed on both iOS and Android, in which case we want to convey that it's a Unity app and the event is firing in a specific platform.

If we have this mutual exclusivity, how can we represent that? I can see at least two options:

  • Allow duplication of states, and rely on the fact that an event can't both be iOS and Android at the same time, but could potentially be one of each, and then also be Unity, React Native, Kotlin Multiplatform, etc.
  • Have a separate (optional) attribute that represents the framework used to generate the app, and keep the actual OS state mutually exclusive.

BTW, this may be beyond the scope of this change - if so, I can submit another pull request to add this.

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 Feb 25, 2024
Copy link

github-actions bot commented Mar 4, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants