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

Exception message override log message #631

Closed
armetiz opened this issue Sep 29, 2023 · 2 comments · Fixed by #632
Closed

Exception message override log message #631

armetiz opened this issue Sep 29, 2023 · 2 comments · Fixed by #632
Assignees
@danielmorell

Comments

@armetiz
Copy link
Contributor

armetiz commented Sep 29, 2023

Hi there.

try {
    throw new \RuntimeException('Not yet implemented.');
} catch (\Throwable $exception) {
    $this->logger->error('User could not be registered.', [
        'exception' => $exception,
    ]);
}

The above Rollbar logged event message is : Not yet implemented.
It should be User could not be registered.
@danielmorell danielmorell self-assigned this Oct 13, 2023
@danielmorell
Copy link
Collaborator

Hey Thomas, thank you for reporting this!

I took a look at the implementation and User could not be registered. is being sent as the description if a Throwable is provided in the 'exception' context. I agree this is probably backwards of the way I would have expected it given your code example above.

Given there are two messages associated with the error, we have three options (at least that I can think of):

  1. Send the Not yet implemented. as the message and User could not be registered. as the description.
  2. Send the User could not be registered. as the message and Not yet implemented. as the description.
  3. Concatenate the two into one message e.g. User could not be registered. (Not yet implemented.).

I think I lean toward option 3. What do you think?

@armetiz
Copy link
Contributor Author

armetiz commented Oct 16, 2023

Hi @danielmorell ,
I think option 2. is the best one! This seems obvious to me because it respects developper intention's.

On the above example, you can see the exception line, but in real application errors are thrown in deeper code. When receiving Rollbar alert, developpers knows about theirs codes, but if message is related to a librairy exception.. It's really hard to understand on first sight.

Option 1 is inappropriate.
Option 3 : this is not a bad idea

danielmorell added a commit that referenced this issue Dec 15, 2023
@danielmorell
Fixed #631 Log message in description
359fe76
danielmorell added a commit that referenced this issue Dec 22, 2023
@danielmorell
Fixed #631 Set log message as primary
59e0360
@danielmorell danielmorell mentioned this issue Dec 22, 2023
Merged
12 tasks
danielmorell added a commit that referenced this issue Dec 22, 2023
@danielmorell
Merge pull request #632 from rollbar/fixed/issue-631-message-in-descr…
72fc135 
…iption

Fixed #631 Set log message as primary
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@danielmorell danielmorell

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixed #631 Set log message as primary rollbar/rollbar-php
2 participants
@armetiz @danielmorell