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: Honeybadger adaptor #19

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

2002Bishwajeet
Copy link

What it does

Fixes #4212

Miscellaneous Changes

  • Changed raw links to the proper markdown page of Contributing and Code of Conduct hyperlinks in add-logger-adaptor.md
  • Minor typo errors

Testing

Manual Testing

Have you read the contributing guidelines?

Always 😉

@2002Bishwajeet
Copy link
Author

The test fails due to the unavailability of the honeybadger API key🙂

@PineappleIOnic
Copy link
Member

Thank you so much for the PR 🤩. We're adding the hacktoberfest-accepted label to ensure this PR counts towards your Hacktoberfest contributions count. With that said, please stay active on this PR to address any comments once you receive a review. Happy Hacktoberfest! 🎃

@stnguyen90 stnguyen90 self-requested a review November 17, 2022 01:19
Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

Great PR! 🤯 We left some comments during the review, please check them out.

tests/LoggerTest.php Show resolved Hide resolved
src/Logger/Adapter/HoneyBadger.php Outdated Show resolved Hide resolved
@christyjacob4
Copy link
Contributor

christyjacob4 commented Jan 10, 2023

@2002Bishwajeet please address the comments

@stnguyen90
Copy link
Contributor

Sample generated from test case:

image

image

Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

Small change please! 🙏🏼

tests/LoggerTest.php Outdated Show resolved Hide resolved
@2002Bishwajeet
Copy link
Author

@stnguyen90 , I did some work on #24. Request for re-review

src/Logger/Adapter/HoneyBadger.php Outdated Show resolved Hide resolved
@christyjacob4
Copy link
Contributor

@2002Bishwajeet thanks a lot for your contributions during Hacktoberfest 2022! Please address fix the failing tests so we can move forward with this.

Meanwhile you can reach out to me on our Discord server if you would like to claim your Appwrite swags! As a way of saying thank you, we would also love to invite you to join the Appwrite organization on GitHub. Please share your GitHub username with us on Discord. 

@2002Bishwajeet
Copy link
Author

Updated changes
image

@2002Bishwajeet
Copy link
Author

Also while running static code analysis, I get this

Checks took 2.54 seconds and used 154.024MB of memory
Psalm was unable to infer types in the codebase

Not sure if it means success or not

Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

Looks like there are some lint errors.

foreach ($breadcrumbsObject as $breadcrumb) {
\array_push($breadcrumbsArray, [
'category' => $breadcrumb->getCategory(),
'timestamp' => \intval($breadcrumb->getTimestamp()),
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this recently, but got an error in Honeybadger because they expect this to be a string like 2020-01-27T12:37:31.616-08:00. Would you please update this to be a ISO-8601 formatted timestamp in UTC?

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.

🚨 Improve Appwrite Logging with HoneyBadger Adapter
4 participants