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

Add mobile crash data model spec #362

Conversation

tonzhan2
Copy link

@tonzhan2 tonzhan2 commented Oct 3, 2023

Tentative draft for mobile crashes reflected as standalone event logs. Not sure if added in the right place, and a little unfamiliar with formatting so any input is appreciated

@AlexanderWert
Copy link
Member

cc @open-telemetry/semconv-mobile-approvers

@tonzhan2 tonzhan2 marked this pull request as ready for review October 10, 2023 18:29
@tonzhan2 tonzhan2 requested review from a team October 10, 2023 18:29
@tonzhan2
Copy link
Author

@tonzhan2 I added a bunch of review comments / proposals on how to refactor the yaml file to be consistent with other semantic conventions.

Apart from that, there are two major things left:

  1. Can you add a readme file (in semantic-conventions/docs/mobile) that would describe the semantic conventions and generate the attributes table in there (see other markdown files for reference).

    • you'll need the <!-- semconv [SEMCONV_ID_GOES_HERE]--> ... <!-- endsemconv --> comments
    • and then run make table-generation
  2. Currently the payload for Android crashes is in exception.* and thread.* attributes, while for iOS it's a single field where the crash content goes in. It might be fine as the data is conceptually different for those systems, but I'd like to here more opinions from @open-telemetry/semconv-mobile-approvers

Will do.
I would defer any questions about the iOS crash payload in the single field, as well as details pertaining to the iOS formatting to my colleague @SMickelsn

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 13, 2024
@github-actions github-actions bot removed the Stale label Feb 21, 2024
@joaopgrassi
Copy link
Member

CC @marandaneto

Comment on lines +36 to +37
brief: >
The contents from PLCrashReporter
Copy link
Member

Choose a reason for hiding this comment

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

Would be possible to make the reporter agnostic?
In case I use KSCrash, I'd like to use the very same data model.
Maybe introducing a new property that tells what's the reporter format, such as KSCrash, PLCrashReporter or anything else.

Comment on lines +9 to +14
- ref: ios.state
requirement_level:
recommended: If the crash occurred on an iOS device.
- ref: android.state
requirement_level:
recommended: If the crash occurred on an Android device.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should just call it state and allow the OS name to be set such as ios or android, at least is flexible enough in case a new OS appears rather than introducing attributes for every OS.

Comment on lines +9 to +11
- ref: ios.state
requirement_level:
recommended: If the crash occurred on an iOS device.
Copy link
Member

Choose a reason for hiding this comment

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

ios is specifically for iPhone, macOS is for MacBook and friends.
Maybe we call it apple instead? since the crash reports share the same format on all Apple platforms.

type: event
brief: >
This document defines attributes for crashes represented using Log Record Events.
`event.domain` is 'device' and `event.name` is 'crash'.
Copy link
Member

@marandaneto marandaneto Mar 4, 2024

Choose a reason for hiding this comment

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

event.name should allow something different than crash - or make it more generic.
crash means the application exited, but this format should allow swallowed errors - but captured as well which is technically not a crash but rather an error/exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

The current thinking is that we will model a separate event type for non-fatal errors/exceptions. There is already an event type called exception in the spec but that's more for span events, that is, errors occuring in the context of a span, but we will need to model a similar event for a more generic usecase.

Copy link
Member

Choose a reason for hiding this comment

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

any reason to make that a different event type? the only difference between a crash and an error is its severity, the metadata is 100% the same.

@tonzhan2 tonzhan2 requested a review from AlexanderWert March 20, 2024 23:29
Copy link

github-actions bot commented Apr 5, 2024

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, 2024
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 13, 2024
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.