Skip to content

Conversation

@yrom
Copy link
Contributor

@yrom yrom commented Sep 2, 2019

ref docs:
transaction: https://docs.sentry.io/development/sdk-dev/event-payloads/#optional-attributes
contexts: https://docs.sentry.io/development/sdk-dev/event-payloads/contexts/
breadcrumbs: https://docs.sentry.io/development/sdk-dev/event-payloads/breadcrumbs/

There is a related pr #40 when i create this pr. In my opinion, the Event class should know nothing about what the contexts have, so i did not create the wrapper class for the os,device,etc which it might contains.

@SammyO
Copy link

SammyO commented Sep 6, 2019

+1 to get this merged

Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

Transaction and breadcrumbs LGTM. Let's leave contexts out of this PR. #40 is adding a fully schemed version of it.

@yrom yrom changed the title Add 'transaction', 'contexts' and 'breadcrumbs' in event Add 'transaction' and 'breadcrumbs' in event Sep 26, 2019
@yrom
Copy link
Contributor Author

yrom commented Sep 26, 2019

@yjbanov The 'contexts' was discarded.

@yjbanov
Copy link
Contributor

yjbanov commented Sep 26, 2019

Thanks, just a couple more things I noticed.

@yrom
Copy link
Contributor Author

yrom commented Sep 27, 2019

@yjbanov Thanks for reviewing. Request changes applied.

@yjbanov
Copy link
Contributor

yjbanov commented Sep 27, 2019

@yrom Curious, why did you decide to rollback the timestamp in cd69392?

@yrom
Copy link
Contributor Author

yrom commented Sep 28, 2019

@jaumard
Copy link

jaumard commented Oct 1, 2019

Would love to have this and "Context" merged, as currently reporting is quite light :)

@yjbanov
Copy link
Contributor

yjbanov commented Oct 1, 2019

@yjbanov Looked up the sentry-java SDK.
See https://github.com/getsentry/sentry-java/blob/master/sentry/src/main/java/io/sentry/marshaller/json/JsonMarshaller.java#L299.
They were using timestamp.

Would you mind trying formatDateAsIso8601WithSecondPrecision instead? This way we'd consistently use the same date-time formatting throughout the client.

@yrom
Copy link
Contributor Author

yrom commented Oct 8, 2019

@yjbanov Sorrry for my late reply. I have updated the commits, again.

@yjbanov
Copy link
Contributor

yjbanov commented Oct 15, 2019

LGTM! Thank you for contributing this feature!

@yjbanov yjbanov merged commit 14e370c into getsentry:master Oct 15, 2019
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.

4 participants