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

Add Fallback to LogEntry #3657

Merged
merged 7 commits into from
Sep 18, 2023
Merged

Conversation

geo2a
Copy link
Collaborator

@geo2a geo2a commented Sep 13, 2023

This PR modifies the LogEntry type in kore-rpc-types in two ways:

  • add origianlTerm into Rewrite log entry constructor (and emit Nothing for now in Kore to retain the current behavior. This change is not necessary at the moment, but we may need to log these terms in future.
  • Add Fallback constructor to LogEntry to enable kore-rpc-booster to emit a trace when it falls back from Booster to Kore.

kore/src/Kore/JsonRpc.hs Outdated Show resolved Hide resolved
@geo2a geo2a force-pushed the georgy/log-term-on-successful-rewrite branch from 048b328 to db8439b Compare September 14, 2023 09:24
@geo2a geo2a changed the title Log term on successful rewrite Allow logging pre-state in Rewrite logs Sep 14, 2023
@geo2a geo2a force-pushed the georgy/log-term-on-successful-rewrite branch from 033fad6 to fc5aa93 Compare September 14, 2023 10:50
@goodlyrottenapple
Copy link
Contributor

originalTermIndex is meant to be a pointer to avoid re-sending large configurations in the logs over and over. not been implemented

@geo2a geo2a force-pushed the georgy/log-term-on-successful-rewrite branch from fc5aa93 to 209a03f Compare September 14, 2023 15:08
Pull-out `Depth` into its own `Kore.JsonRpc.Types.Depth` module
@geo2a geo2a changed the title Allow logging pre-state in Rewrite logs Add Fallback to LogEntry Sep 15, 2023
@@ -46,7 +48,8 @@ data LogRewriteResult

data LogEntry
= Rewrite
{ result :: LogRewriteResult
{ originalTerm :: Maybe KoreJson
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need the original term? the intention for the rewrite trace was to be used when debugging proofs with single step turned on in pyk, so we already have the originalTerm anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, let's drop it after all.

Comment on lines 61 to 75
| -- | Indicates a fallback of an RPC-server to a more powerful, but slower backup server, i.e. Booster to Kore
Fallback
{ originalTerm :: Maybe KoreJson
-- ^ state before fallback
, rewrittenTerm :: Maybe KoreJson
-- ^ state after fallback
, reason :: Text
-- ^ fallback reason
, ruleIds :: NonEmpty Text
-- ^ rules applied during fallback, the first rule is the one that caused the fallback
, recoveryDepth :: Depth
-- ^ depth reached in fallback, must be the same as (length ruleIds)
, origin :: LogOrigin
-- ^ proxy server the log was emitted from
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks good. should have all the info we need :)

Copy link
Member

@jberthold jberthold left a comment

Choose a reason for hiding this comment

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

LGTM (Changes many of the same places as #3658 ... let's get it in and move on)

@rv-jenkins rv-jenkins merged commit 85b5d08 into master Sep 18, 2023
9 checks passed
@rv-jenkins rv-jenkins deleted the georgy/log-term-on-successful-rewrite branch September 18, 2023 08:33
rv-jenkins pushed a commit to runtimeverification/hs-backend-booster that referenced this pull request Sep 27, 2023
…alls back to Kore (#313)

Closes
#295

Needs runtimeverification/haskell-backend#3657

Note that the `Fallback` entry changes its level of detail in presence
of `logSuccessfulRewrites`/logFailedRewrites`:
* If only `logFallbacks` is supplied, then the log entry will only
contain the pre- and post-state
* If `logFailingRewtires` is given too, the log entry will additionally
contain the rule id that caused the fallback and the fallback reason
* If `logSuccessfulRewrites` is given too, the log entry will
additionally contain the the rule log in Kore
jberthold pushed a commit that referenced this pull request Apr 10, 2024
…alls back to Kore (#313)

Closes
runtimeverification/hs-backend-booster#295

Needs #3657

Note that the `Fallback` entry changes its level of detail in presence
of `logSuccessfulRewrites`/logFailedRewrites`:
* If only `logFallbacks` is supplied, then the log entry will only
contain the pre- and post-state
* If `logFailingRewtires` is given too, the log entry will additionally
contain the rule id that caused the fallback and the fallback reason
* If `logSuccessfulRewrites` is given too, the log entry will
additionally contain the the rule log in Kore
jberthold pushed a commit that referenced this pull request Apr 10, 2024
…alls back to Kore (#313)

Closes
runtimeverification/hs-backend-booster#295

Needs #3657

Note that the `Fallback` entry changes its level of detail in presence
of `logSuccessfulRewrites`/logFailedRewrites`:
* If only `logFallbacks` is supplied, then the log entry will only
contain the pre- and post-state
* If `logFailingRewtires` is given too, the log entry will additionally
contain the rule id that caused the fallback and the fallback reason
* If `logSuccessfulRewrites` is given too, the log entry will
additionally contain the the rule log in Kore
jberthold pushed a commit that referenced this pull request Apr 10, 2024
…alls back to Kore (#313)

Closes
runtimeverification/hs-backend-booster#295

Needs #3657

Note that the `Fallback` entry changes its level of detail in presence
of `logSuccessfulRewrites`/logFailedRewrites`:
* If only `logFallbacks` is supplied, then the log entry will only
contain the pre- and post-state
* If `logFailingRewtires` is given too, the log entry will additionally
contain the rule id that caused the fallback and the fallback reason
* If `logSuccessfulRewrites` is given too, the log entry will
additionally contain the the rule log in Kore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants