8000 Fix for 'data' key in logger's extra by gagan-chawla · Pull Request #337 · getsentry/sentry-python · GitHub
[go: up one dir, main page]
Skip to content

Fix for 'data' key in logger's extra #337

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
Merged
merged 3 commits into from
Apr 15, 2019
Merged

Fix for 'data' key in logger's extra #337

merged 3 commits into from
Apr 15, 2019

Conversation

gagan-chawla
Copy link
Contributor

Fixes #336

@gagan-chawla gagan-chawla mentioned this pull request Apr 13, 2019
Closed
@untitaker
Copy link
Member

Can we preserve the current behavior in this SDK? I feel like Raven's old behavior added an unnecessary layer of complexity (accessing data first), but maybe I am missing something.

I would've expected that you just remove the data key from the const.

@gagan-chawla
Copy link
Contributor Author

Can we preserve the current behavior in this SDK? I feel like Raven's old behavior added an unnecessary layer of complexity (accessing data first), but maybe I am missing something.

I would've expected that you just remove the data key from the const.

I thought of just removing data from COMMON_RECORD_ATTRS before, but keeping Raven's old behaviour in mind, this way we'd unwrap data dict before showing it in sentry's Additional Data section.

If we still wanna preserve the current behaviour, I guess removing data from constant also resolves the issue.

@untitaker
Copy link
Member

So what I want to understand is why you prefer Raven's behavior over sentry-sdk's behavior. I am open to either one, but we have to keep in mind that changing this behavior is a breaking change within sentry-sdk's version timeline.

@gagan-chawla
Copy link
Contributor Author

As I migrated from raven to new sentry-sdk, my projects stopped logging info as I was following old Raven's way to log additional data. This change is what I did to kinda cope with it.

I guess Raven did unwrapping of data to make it easier to integrate logger's components-
Since Raven provided two main extended functionalities to logging - 'extra' = {data: {...}, stack: True}, so it was convenient to maintain a data dict (that also performed unwrapping similar to 'extra') and to explicitly mention stack True when required, rather than adding stack key along with other data in a dict and pass it to 'extra' directly.

But as you mentioned earlier, exc_info is used for callstack in sentry-sdk, I think unwrapping data doesn't serve any purpose now. I'm inclined towards removing data from constant as well.

@untitaker
Copy link
Member

Good point. I agree and would prefer to keep sentry-sdk's current behavior wrt data unwrapping.

@untitaker untitaker merged commit aba45f4 into getsentry:master Apr 15, 2019
@untitaker
Copy link
Member

Thanks!

< 6593 /form>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unified sentry-sdk integration does not have support to add stack trace in python logger using 'stack': True in extra dict.
2 participants
@gagan-chawla @untitaker
0