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.

Sign up for GitHub

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

Jump to bottom

Allow populating breadcrumb timestamp with custom value #1193

Merged
merged 2 commits into from
Mar 15, 2021
Merged

Allow populating breadcrumb timestamp with custom value #1193

merged 2 commits into from
Mar 15, 2021

Conversation

romainneutron
Copy link
Contributor

No description provided.

Jean85
Jean85 reviewed Mar 9, 2021
View reviewed changes
Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Hello, and thank you for your contribution.

Can you tell us why you would need such a feature?

PS: CI failure seems unrelated.

@ste93cry ste93cry changed the base branch from master to develop March 9, 2021 14:37
@romainneutron
Copy link
Contributor Author

Hello,

We use sentry PHP in a legacy app that has its own monitoring system. Once an error happens, we create sentry event and we convert our internal breadcrumbs to sentry breadcrumbs.
At the moment, those breadcrumbs all have the same timestamp since they are created lately. Allowing setting the breadcrumb timestamp will allow us to properly set the time the breadcrumb happened

@ste93cry ste93cry added this to the 3.3 milestone Mar 9, 2021
@Jean85 Jean85 mentioned this pull request Mar 9, 2021
Merged
ste93cry
ste93cry requested changes Mar 9, 2021
View reviewed changes
Copy link
Collaborator

@ste93cry ste93cry left a comment

Choose a reason for hiding this comment

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

Can you please add a CHANGELOG entry and also both the method withTimestamp() and the related test method?

src/Breadcrumb.php Show resolved Hide resolved
tests/BreadcrumbTest.php Outdated Show resolved Hide resolved
@romainneutron romainneutron force-pushed the breadcrumb-timestamp branch 3 times, most recently from ee453a4 to 5e27985 Compare March 10, 2021 07:34
@romainneutron
Copy link
Contributor Author

PR updated, comments addressed

Jean85
Jean85 requested changes Mar 10, 2021
View reviewed changes
Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Changelog entry still missing; code LGTM, but we're still getting that strange error on PHP 8 + Windows

@romainneutron
Copy link
Contributor Author

PR updated, comments addressed

ste93cry
ste93cry approved these changes Mar 12, 2021
View reviewed changes
Copy link
Collaborator

@ste93cry ste93cry left a comment

Choose a reason for hiding this comment

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

Hopefully, you don't mind that I took the liberty to update some things to either improve them or align them to the coding standard of the project

@ste93cry ste93cry merged commit 61eed66 into getsentry:develop Mar 15, 2021
@romainneutron romainneutron deleted the breadcrumb-timestamp branch April 30, 2021 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ste93cry ste93cry ste93cry approved these changes

@Jean85 Jean85 Jean85 approved these changes

Assignees
No one assigned
Labels
Type: Improvement
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@romainneutron @ste93cry @Jean85