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

feat: Add context data to events #63

Merged
merged 11 commits into from
Apr 16, 2018
Merged

feat: Add context data to events #63

merged 11 commits into from
Apr 16, 2018

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented Apr 12, 2018

The operating system version is still wrong and I think we can do a better job in getting the operating system name (e.g. via user agent).

Fixes #37
Fixes #40

@jan-auer jan-auer requested review from HazAT and timfish April 13, 2018 14:03
@jan-auer
Copy link
Member Author

To properly display those contexts, we still need some changes in the Sentry Context UI. Especially, if we want to provide consistent rendering - see #40 for more details.

src/frontend.ts Outdated
scope: Scope,
): Promise<SentryEvent> {
const prepared = await super.prepareEvent(event, scope);
return addEventDefaults(prepared);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What stops this being called from the renderer process?

All the app and process access in context.ts will only work from the main process.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually its protected and throws from sendEvent so this should be fine

Copy link
Member Author

Choose a reason for hiding this comment

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

We can add a guard and throw in case someone tries to invoke it 👍

Copy link
Collaborator

@timfish timfish left a comment

Choose a reason for hiding this comment

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

Were you also planning to delete server_name in this PR?

@jan-auer
Copy link
Member Author

Updated, this should be ready to merge now.

@jan-auer jan-auer merged commit 20d9658 into master Apr 16, 2018
@jan-auer jan-auer deleted the feat/context branch April 16, 2018 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants