[TSD-70] Exception handling when reading log files #26

HirotoShioi · 2018-07-09T09:01:17Z

This PR will introduce exception handling on reading log files so that the program will not crash due to the corrupted log file.

Fix test cases (Don't use external file when testing)
Exception handling on reading log files. (Example tciket id: 15066)

https://iohk.myjetbrains.com/youtrack/issue/TSD-70

ksaric · 2018-07-09T13:41:49Z

src/Lib.hs

 -- | Given number of file of inspect, knowledgebase and attachment,
 -- analyze the logs and return the results.
-inspectAttachment :: Config -> TicketInfo -> AttachmentContent -> ZendeskResponse
+inspectAttachment :: (MonadCatch m) => Config -> TicketInfo -> AttachmentContent -> m ZendeskResponse


Don't touch this, this is all pure, no need to infect that part of the code with impurities.

ksaric · 2018-07-09T13:47:04Z

src/Lib.hs

    att             <- MaybeT $ getAttachment lastAttachment

-    pure $ inspectAttachment config ticketInfo att
+    catchAny (inspectAttachment config ticketInfo att)


We are not looking for Any exception, we need specific exceptions we can throw and catch. Like here - https://github.com/input-output-hk/cardano-sl/blob/feature/CBR-163/wallet-new/src/Cardano/Wallet/WalletLayer/Error.hs

Thanks for the advice! Will look into it!

HirotoShioi · 2018-07-10T08:43:28Z

Implemented obvious ones. Now I'm going to try to solve exception *** Exception: Codec.Compression.Zlib: premature end of compressed data stream

ksaric · 2018-07-11T09:11:19Z

src/Lib.hs

 -- analyze the logs and return the results.
-inspectAttachment :: Config -> TicketInfo -> AttachmentContent -> ZendeskResponse
-inspectAttachment Config{..} ticketInfo@TicketInfo{..} attContent = do
+inspectAttachment :: Config -> TicketInfo -> [LByteString]-> ZendeskResponse


Why did you strip the newtype wrapper?

Well, previously, we were passing raw zip file asAttachmentContent whereas I've changed it to pass the decompressed log files.
The only way I found of making this function pure is to decompress the file before passing it instead of decompressing it inside of this function.

ksaric · 2018-07-11T09:11:49Z

src/DataSource/Types.hs

             , MonadReader Config
             , MonadIO
             , MonadBase IO
+             , MonadCatch


I really think we don't need this, unless you want to re-export, but that's a whole different story.

Ah, sorry, I thought that was from the module exports 😁
Should have looked in more detail.

ksaric · 2018-07-11T09:12:42Z

log-classifier.cabal

                     , optparse-applicative
                     , reflection
                     , regex-tdfa
+                     , safe-exceptions


Are you sure we need this? http://hackage.haskell.org/package/universum-1.2.0/docs/Universum-Exception.html

I've tried importing tryDeep from Universum.Exceptions but didn't work. I'm not sure what they mean by re-export. Perhaps it's because we're using the older version of universum (1.1.0)
cardano-sl 's wallet-new component is importing safe-exceptions even though it's using universum as well.
https://github.com/input-output-hk/cardano-sl/blob/develop/wallet-new/cardano-sl-wallet-new.cabal#L175

Maybe there's some way to import it and I just don't know how.

Ok, keep it there, just try using functions from Universum as much as possible.

ksaric · 2018-07-11T09:14:01Z

src/Lib.hs

    postTicketComment   <- asksZendeskLayer zlPostTicketComment
-
+
+    -- What should we do if the zendeskResponse was Nothing?


The point of this refactoring is to add error details and remove Maybe where possible. Maybe you would have an idea what to do with it if you had specific exceptions you could catch here?

Ah okay, I'd tackle on that part once I'm done with handling the decompression issue..

ksaric · 2018-07-11T09:16:21Z

src/Util.hs

+-- Why tryDeep instead of try? It's because we need to fully evaluate bytestrings in order to catch
+-- the decompression issue.
+readZip :: (MonadCatch m, MonadIO m) => LByteString -> m (Map FilePath LByteString)
 readZip rawzip = case Zip.toArchiveOrFail rawzip of


Did you try it out? Does it catch the exceptions?

Yes, I did try to use try at first since it has less constraints but it didn't catch the decompression error.
I can try again.

ksaric · 2018-07-17T08:25:52Z

src/DataSource/Types.hs

      coComment :: Comment
    }

+newtype LogFiles = LogFiles {


I prefer the formatting seen here - https://github.com/input-output-hk/log-classifier/pull/26/files#diff-ee661ebeb72d3f09577f0fd782917c5aR359 since it doesn't change any other lines besides the ones you change which is helpful.

ksaric · 2018-07-17T08:46:02Z

test/Spec.hs

+                        -- Check we have some comments.
+                        assert $ isLeft (eZendeskResponse :: Either ProcessTicketExceptions ZendeskResponse)
+                        whenLeft eZendeskResponse $ \processException ->
+                            assert $ processException == TicketInfoNotFound ticketId


ksaric · 2018-07-17T08:54:18Z

src/Lib.hs

-    att             <- MaybeT $ getAttachment lastAttachment
-
-    pure $ inspectAttachment config ticketInfo att
+    mAtt <- runMaybeT $ do


This seems overly verbose. Can you move it around a bit to make it a bit more concise?

I'll try it out. Thank you.

ksaric · 2018-07-17T08:55:04Z

test/Spec.hs

+                    -- Check it throws exception
+                    -- How do I check if it returns proper exception?
+                    assert $ isLeft (eZendeskResponse :: Either ProcessTicketExceptions ZendeskResponse)
+                    whenLeft eZendeskResponse $ \processException ->


…tion

ksaric

LGTM.

ksaric · 2018-07-19T13:10:40Z

src/Lib.hs

-    att             <- MaybeT $ getAttachment lastAttachment
-
-    pure $ inspectAttachment config ticketInfo att
+    lastAttach <- handleMaybe (safeHead . reverse . sort $ attachments)


handleMaybe . safeHead . reverse . sort $ attachments

ksaric · 2018-07-19T13:12:16Z

test/Spec.hs


                        -- Check we have some comments.
-                        assert $ isJust zendeskResponse
+                        assert $ (not . null . zrComment) zendeskResponse


assert . not . null . zrComment $ zendeskResponse

mdimjasevic

So far I finishing reading everything but the tests, which looks good. I'll finish with tests tomorrow.

mdimjasevic · 2018-07-19T12:10:39Z

src/Lib.hs

+    -- No attachment, no comments means something is wrong with ticket itself

 -- | Inspect only the latest attachment. We could propagate this
 -- @Maybe@ upwards or use an @Either@ which will go hand in hand


Shouldn't the documentation of the inspectAttachments function be updated now that the return type is not App (Maybe ZendeskResponse) anymore?

Good catch! I'll update it.

mdimjasevic

I with the test/Spec.hs was better organised: it looks like there is a lot of repetition of very similar code.

Other than that, this looks good to me.

HirotoShioi added the WIP label Jul 9, 2018

ksaric reviewed Jul 9, 2018

View reviewed changes

HirotoShioi changed the title ~~[TSD-79] Exception handling when reading log files~~ [TSD-70] Exception handling when reading log files Jul 10, 2018

HirotoShioi force-pushed the hiroto/TSD-79-exception-handling branch 2 times, most recently from 7b41e15 to 54d39b7 Compare July 11, 2018 05:07

ksaric reviewed Jul 11, 2018

View reviewed changes

HirotoShioi force-pushed the hiroto/TSD-79-exception-handling branch 2 times, most recently from 63d68e1 to 8a6cb8f Compare July 17, 2018 07:56

ksaric reviewed Jul 17, 2018

View reviewed changes

HirotoShioi removed the WIP label Jul 17, 2018

HirotoShioi force-pushed the hiroto/TSD-79-exception-handling branch 3 times, most recently from 3e0add1 to f219ec7 Compare July 18, 2018 06:37

HirotoShioi added 15 commits July 19, 2018 18:30

[TSD-70] Fix test cases

1857a4c

Exception handling when reading log files

fde8aa1

Exception handling on reading zip file

16631a0

[TSD-70] Exception handling on decompression failure

ca568ff

[TSD-70] Minor fixes

7efcac0

[TSD-70] Implement exception handling on ticket exceptions

b9a7a03

[TSD-70] Small improvements

24cdd25

[TSD-70] Add show instance of processticket exception

92b1ea8

[TSD-70] Fix test cases, add cases to check if it throws proper excep…

3703b1a

…tion

Change function name

5bd8f8a

Make exception message more specific, fix text case messages

cefab33

Remove comment

edded55

[TSD-70] Exception handling within analysis module

d62103a

[TSD-70] Small refactoring

e991072

Catch exception without using tryDeep

3854e93

Use ! to force evaluation

f179118

HirotoShioi force-pushed the hiroto/TSD-79-exception-handling branch from e0f0ae4 to f179118 Compare July 19, 2018 09:32

[TSD-79] Cosmetic fix for readability.

6df6932

ksaric approved these changes Jul 19, 2018

View reviewed changes

mdimjasevic reviewed Jul 19, 2018

View reviewed changes

[TSD-70] Small improvement to the code

5a7719a

HirotoShioi force-pushed the hiroto/TSD-79-exception-handling branch from 27c6092 to 5a7719a Compare July 20, 2018 03:13

mdimjasevic approved these changes Jul 20, 2018

View reviewed changes

HirotoShioi merged commit b1feef4 into develop Jul 20, 2018

HirotoShioi deleted the hiroto/TSD-79-exception-handling branch August 2, 2018 04:09

		postTicketComment <- asksZendeskLayer zlPostTicketComment


		-- What should we do if the zendeskResponse was Nothing?

[TSD-70] Exception handling when reading log files #26

[TSD-70] Exception handling when reading log files #26

Uh oh!

Conversation

HirotoShioi commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HirotoShioi commented Jul 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HirotoShioi Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HirotoShioi Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksaric left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdimjasevic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdimjasevic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HirotoShioi commented Jul 9, 2018 •

edited

Loading

HirotoShioi Jul 12, 2018 •

edited

Loading

HirotoShioi Jul 12, 2018 •

edited

Loading