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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Do not include stacktrace into Timber message #1898

Merged
merged 10 commits into from Feb 8, 2022
Merged

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Feb 3, 2022

馃摐 Description

  • Overridden all Timber.Tree methods to have full control over the log message
  • Supply formatted and params fields if the log-call was made with args
  • Do not include stackframes into a message

How this looks now:
image

Grouping seems to work as well:
image

馃挕 Motivation and Context

Closes #1177

馃挌 How did you test it?

馃摑 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

馃敭 Next steps

  1. We can't access tag from Timber because it's package-private, so I had to drop it.
  2. Since we set the Message ourselves, the stacktraces are not reported as breadcrumbs anymore (previously, Timber default implementation would put a stacktrace into a message variable and we would record a breadcrumb with the exception) - should we still do this?

@marandaneto
Copy link
Contributor

Since we set the Message ourselves, the stacktraces are not reported as breadcrumbs anymore (previously, Timber default implementation would put a stacktrace into a message variable and we would record a breadcrumb with the exception) - should we still do this?

No, the breadcrumb should not care about the stack trace, if there's an exception, we'll capture an event anyway.

@marandaneto
Copy link
Contributor

marandaneto commented Feb 3, 2022

We can't access tag from Timber because it's package-private, so I had to drop it.

That's a breaking change but not sure if it makes any difference or if somebody uses it as a filter on the Issues page.
@bruno-garcia ?

@romtsn
Copy link
Member Author

romtsn commented Feb 4, 2022

That's a breaking change but not sure if it makes any difference or if somebody uses it as a filter on the Issues page. @bruno-garcia ?

Yeah, timber isn't great when it comes to setting tags, because you need to invoke a separate method Timber.tag before calling the actual logging function. For DebugTree it infers the tag from a class name, in theory we could do the same if desired.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

鉂わ笍

@romtsn romtsn marked this pull request as ready for review February 7, 2022 09:14
@romtsn
Copy link
Member Author

romtsn commented Feb 7, 2022

So the way to go with Timber I think we just do not support tags for now, if we get complaints/issues we can offer a reflection-based support (or automatically infer the tag from the classname). How does that sound?

@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2022

Codecov Report

Merging #1898 (2df5bb6) into main (3cd777f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #1898   +/-   ##
=========================================
  Coverage     75.42%   75.42%           
  Complexity     2244     2244           
=========================================
  Files           225      225           
  Lines          8026     8026           
  Branches        852      852           
=========================================
  Hits           6054     6054           
  Misses         1562     1562           
  Partials        410      410           

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 3cd777f...2df5bb6. Read the comment docs.

@romtsn
Copy link
Member Author

romtsn commented Feb 7, 2022

We could also mention it in the docs and link to an issue on github to vote for

@marandaneto
Copy link
Contributor

marandaneto commented Feb 7, 2022

We could also mention it in the docs and link to an issue on github to vote for

Let's do this, but let's also add the missing tags feat. on the changelog entry.

@romtsn
Copy link
Member Author

romtsn commented Feb 8, 2022

Added a breaking change note on the changelog and linked it to a gh issue. Merging it when it's green

@romtsn romtsn enabled auto-merge (squash) February 8, 2022 11:44
@romtsn romtsn merged commit af2b1a3 into main Feb 8, 2022
@romtsn romtsn deleted the fix/timber-grouping branch February 8, 2022 11:55
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.

Timber: Don't include stack trace frames in message
4 participants