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

Added Airbrake Adapter #18

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

Conversation

devblin
Copy link

@devblin devblin commented Oct 13, 2022

@devblin
Copy link
Author

devblin commented Oct 13, 2022

@christyjacob4 The CI is failing as there are no test ENVs provided.

@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
Copy link
Contributor

It does look like the data is pushed to airbrake, but it seems like a lot of data is missing like:

  • stack trace
  • bread crumbs
  • tags
  • extras

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.

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

Comment on lines +94 to +101
foreach ($breadcrumbObjects as $breadcrumbObject) {
$error = [
"type" => $breadcrumbObject->getType(),
"message" => $breadcrumbObject->getMessage()
];

\array_push($errors, $error);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding the breadcrumbs to errors, I think it should come from $log->getExtra()['detailedTrace']. See

if (isset($log->getExtra()['detailedTrace'])) {
foreach ($log->getExtra()['detailedTrace'] as $trace) {
\array_push($stackFrames, [
'filename' => $trace['file'],
'lineno' => $trace['line'],
'function' => $trace['function'],
]);
}
}

Copy link
Author

Choose a reason for hiding this comment

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

Hey @stnguyen90 , sorry for being inactive here and responding so late.
Looking at your comment and the airbrake docs, I think the detailedTrace should go in error/{i}/backtrace as per the doc

Copy link
Author

Choose a reason for hiding this comment

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

I am suggesting this change:

Suggested change
foreach ($breadcrumbObjects as $breadcrumbObject) {
$error = [
"type" => $breadcrumbObject->getType(),
"message" => $breadcrumbObject->getMessage()
];
\array_push($errors, $error);
}
foreach ($breadcrumbObjects as $breadcrumbObject) {
$backtraces = [];
foreach ($detailedTraces as $detailedTrace) {
$backtrace = [
"file" => $detailedTrace['file'],
"line" => $detailedTrace['line'],
"function" => $detailedTrace['function']
];
\array_push($backtraces, $backtrace);
}
$error = [
"type" => $breadcrumbObject->getType(),
"message" => $breadcrumbObject->getMessage(),
"backtrace" => $backtraces
];
\array_push($errors, $error);
}

@christyjacob4
Copy link
Contributor

@devblin please address the comments

@stnguyen90 stnguyen90 self-requested a review March 1, 2023 02:40
@christyjacob4
Copy link
Contributor

@devblin thanks a lot for your contributions during Hacktoberfest 2022!

Please 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. 

Also let us know if you'd like to continue working on this PR.

@devblin
Copy link
Author

devblin commented Apr 6, 2023

Hey @christyjacob4 , I am unable to accept the discord server invite.
image

@devblin
Copy link
Author

devblin commented Apr 6, 2023

Also, @christyjacob4 can you please address my comment on the suggested change for this comment ?.

@devblin
Copy link
Author

devblin commented Apr 6, 2023

@christyjacob4 I would like to continue working on this PR, I was waiting for the response from the team 😅

@christyjacob4
Copy link
Contributor

Screenshot 2023-04-06 at 2 56 49 PM

@devblin the link seems to work for me. Can you try this as well?

@devblin
Copy link
Author

devblin commented Apr 6, 2023

@christyjacob4 I remember, I was blocked from server due to unwanted spam messages from my discord id. Can you please check if I am banned ?

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 merge conflicts.

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 Airbrake Adapter
4 participants