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

Filtering stack trace #144

Merged
merged 3 commits into from
May 20, 2020

Conversation

lightning95
Copy link
Contributor

@lightning95 lightning95 commented May 17, 2020

That's the first solution to #137.
But I have several concerns about it:

  1. IterableOnceCompat doesn't look like a very elegant solution for 2.12-2.13 compatibility.
  2. here full stack trace is filtered even if only one row is needed (e.g. Depth.Fixed(1)), but to optimize it, it would require something like this
      val stackTrace = (depth, filter) match {
        case (None, None) => t.getStackTrace
        case (Some(depthLimit), None) => t.getStackTrace.take(depthLimit)
        case (None, Some(set)) => t.getStackTrace.filterNot(row => set.contains(row.getClassName.stripSuffix("$")))
        case (Some(depthLimit), Some(set)) => t.getStackTrace
          .to(LazyList)
          .filterNot(row => set.contains(row.getClassName.stripSuffix("$")))
          .take(depthLimit)
          .toArray
      }

And again, here (t.getStackTrace.to(LazyList)) requires 2.12-2.13 compatibility.

add "ThrowableFormat.Filter" with options to NoFilter" and "Excluding"
add test for "Filter"
add example
@codecov
Copy link

codecov bot commented May 17, 2020

Codecov Report

Merging #144 into master will decrease coverage by 0.12%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
- Coverage   93.33%   93.20%   -0.13%     
==========================================
  Files          32       32              
  Lines         465      471       +6     
  Branches        9       12       +3     
==========================================
+ Hits          434      439       +5     
- Misses         31       32       +1     
Flag Coverage Δ
#unittests 93.20% <87.50%> (-0.13%) ⬇️
Impacted Files Coverage Δ
...la/io/odin/formatter/options/ThrowableFormat.scala 50.00% <50.00%> (-50.00%) ⬇️
...e/src/main/scala/io/odin/formatter/Formatter.scala 98.59% <100.00%> (+0.10%) ⬆️

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 80bed4f...8c5967c. Read the comment docs.

@lightning95 lightning95 marked this pull request as ready for review May 17, 2020 11:28
Copy link
Contributor

@sergeykolbasov sergeykolbasov left a comment

Choose a reason for hiding this comment

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

Hi @lightning95

Thanks for PR, it looks very promising! Good to see that you care about performance. I have couple of comments though


object Excluding {
def apply(prefixes: String*): Excluding = new Excluding(prefixes.toSet)
def apply(prefixes: IterableOnceCompat[String]): Excluding = new Excluding(prefixes.toSetCompat)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of going so generic here? It adds unneccessary complexity for benefits I'm clearly missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, users may want to construct ThrowableFormat.Filter.Excluding from any Traversable (Iterable)
e.g. read from typesafe config and create logging format, having to do something like this:
Filter.Excluding(config.getStringList("exclude"): _ *)
or
Filter.Excluding(config.getStringList("exclude").toSet)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine for the end user to convert whatever they have to the set on their own

writeStackTrace(builder, depth.fold(t.getStackTrace)(t.getStackTrace.take), indent)

val filteredStackTrace = filter.fold(t.getStackTrace)(set =>
t.getStackTrace.filterNot(row => set.contains(row.getClassName.stripSuffix("$")))
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be faster to move this check into writeStackTrace loop and save few allocations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then we'll need to move the t.getStackTrace.take part into writeStackTrace loop too, because we could take(5) and then filter them out, having empty list of rows.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, it's possible to just count amount of actual lines printed and stop whenever depth is achieved

no worries, it's a matter of optimization in separate PR then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I make this PR for optimization?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lightning95 sorry haven't noticed it earlier

yes, feel free to

Copy link
Contributor

@sergeykolbasov sergeykolbasov left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @lightning95

I'm going to merge it, update the documentation and land the release soon

@sergeykolbasov sergeykolbasov merged commit 1abbecf into valskalla:master May 20, 2020
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.

2 participants