debug: Adding debugger #6877

johanfylling · 2024-07-18T12:05:57Z

Fixes: open-policy-agent#6876 Signed-off-by: Johan Fylling <[email protected]>

(mostly renaming) Signed-off-by: Johan Fylling <[email protected]>

Signed-off-by: Johan Fylling <[email protected]>

charlieegan3

This a tremendous effort Johan! I've tested this via the Regal PR and seen it work. Most of my comments are related to comments and documentation, please feel free to take or leave as you please.

Amazing work! 👏

topdown/query.go

topdown/cache.go

debug/variable.go

rego/rego.go

debug/debugger.go

debug/latch.go

debug/debugger.go

Signed-off-by: Johan Fylling <[email protected]>

ashutosh-narkar

This looks great. I had a high-level question about the direction of this change. From #6876 I read this as OPA providing a debugger interface for applications to leverage but it looks like we're adding a debugger implementation in OPA. Did I miss something about the intent behind this change?

johanfylling · 2024-07-24T19:32:59Z

@ashutosh-narkar , perhaps the ticket is a bit sparse and poorly worded, but the intent has always been to create a debugger in OPA that 3rd parties can interface with through the SDK.

The point at which to contest this would of course be now, before this becomes a part of OPA proper.

I think it makes sense to have this be a tightly integrated component, as it'll make it less likely changes to OPA will break the debugger. And as history tells us, even though most of this functionality is possible as a stand-alone project, it's very unlikely that any 3rd party outside of the OPA team will develop and maintain this.

Signed-off-by: Johan Fylling <[email protected]>

ashutosh-narkar · 2024-07-24T22:21:06Z

perhaps the ticket is a bit sparse and poorly worded, but the intent has always been to create a debugger in OPA that 3rd parties can interface with through the SDK.

Thanks for the clarification. I thought we are creating an interface for a debugger which 3rd parties can hook up their own implementations.

And as history tells us, even though most of this functionality is possible as a stand-alone project, it's very unlikely that any 3rd party outside of the OPA team will develop and maintain this.

That's probably true. Then the question is why can't this be a stand-alone project that the OPA team maintains, like the OPA-Envoy plugin for example. If the team is maintaining it, we'd anyways keep it updated with the latest OPA changes irrespective of where it lives. Also in terms of external contributors who want to contribute to tooling for example, this would be a good one to get started with. If we're getting some performance/usage/implementation benefits by keeping this in OPA then it makes sense to have it tightly coupled. Otherwise my suggestion would be to define the interface in OPA and have a mechanism to hook this in.

johanfylling · 2024-07-25T13:07:27Z

Implementation-wise, the benefit is access to internal packages and easier/quicker turnover for fixes and new features that require changes to OPA, as we don't have a secondary project we need to either stagger releases for or keep up-to-date during release cycles. E.g. if we make a change in OPA that has adverse effects for the debugger, then we wouldn't know until the debugger tests are run on the new changes. This is of course possible to automate, but it's a bit cumbersome to maintain.

Usage-wise, 3rd parties that integrate with the debugger would have yet another dependency to keep track of (arguably a minor detail). Discoverability is another issue. If a user has issues with the debugger, they'll probably file a bug report with OPA first, which we'll need to refer over to the debugger project.

By themselves, these might not be super strong arguments against a separate project, but at the same time, I don't think it's a gross feature creep to have the project that's responsible for compiling and executing a language to also have the feature to debug it.

anderseknert · 2024-08-12T08:43:11Z

Then the question is why can't this be a stand-alone project that the OPA team maintains, like the OPA-Envoy plugin for example. If the team is maintaining it, we'd anyways keep it updated with the latest OPA changes irrespective of where it lives.

That's still another thing we'd need to remember to do, and actually do, every time there's a new OPA release. Considering our limited resources, I think it makes sense to include this in OPA core for the sake of minimizing maintainer burden. I'm sure there are technical/performance benefits of that too, but others are better qualified to judge that.

Signed-off-by: Johan Fylling <[email protected]>

ashutosh-narkar

This is great work! I haven't gone through the whole PR yet but will do that in this week. Adding some comments for the parts I've looked at so far.

ashutosh-narkar · 2024-08-20T18:21:32Z

debug/breakpoint.go

+		var newBps breakpointList
+		for _, bp := range bps {
+			if bp.ID() != id {
+				newBps = append(newBps, bp)
+			} else {
+				removed = bp
+			}
+		}
+		bc.breakpoints[path] = newBps
+	}


Did you consider using a linked list for storing the breakpoints. I don't think performance is priority here even if this becomes too large but probably would make the operation cleaner.

Are you suggesting to change the root container from a map to a linked list, or only the array value?
We're diving into this collection quite often to find breakpoints, so it makes sense to do a first lookup by file, even though the number of breakpoints are unlikely to ever get extremely high.

even though the number of breakpoints are unlikely to ever get extremely high.

Doesn't matter then. All good.

ashutosh-narkar · 2024-08-20T18:23:23Z

debug/breakpoint.go

+			if i > 0 {
+				buf.WriteString(", ")
+			}
+			_, _ = fmt.Fprintf(buf, "%s:%d\n", path, bp.Location().Row)


Could we just use the stringer on the breakpoint or there is some reason behind not including the id?

ashutosh-narkar · 2024-08-20T18:27:21Z

debug/debugger.go

+	ResumeAll() error
+
+	// StepOver executes the next expression in the current scope and then stops on the next expression in the same scope,
+	// not stopping on expressions un sub-scopes; e.g. execution of referenced rule, called function, comprehension, or every expression.


ashutosh-narkar · 2024-08-20T18:36:40Z

debug/debugger.go

+	SetBreakpoints(locations []location.Location) ([]Breakpoint, error)
+
+	// AddBreakpoint sets a breakpoint at the given location.
+	AddBreakpoint(loc location.Location) (Breakpoint, error)


Nit: let's stick with AddBreakpoint(s) or SetBreakpoint(s)

Yeah, I think we can drop SetBreakpoints 👍

ashutosh-narkar · 2024-08-20T18:39:54Z

debug/debugger.go

+	// Terminate stops all threads in the session.
+	Terminate() error
+
+	// TODO: Add Stop(ThreadID) func for stopping (pausing) a thread's execution.


Should we remove this or are you planning to make it part of the current work? If not, we can remove and open an issue to track this.

ashutosh-narkar · 2024-08-20T18:54:58Z

debug/debugger.go

+		return nil, fmt.Errorf("failed to prepare query for evaluation: %v", err)
+	}
+
+	if err := store.Commit(ctx, txn); err != nil {


What are we committing here?

We're committing any data added during PrepareForEval. I found that if we don't do this, then we won't be able to read this data from the store in thread.dataVars(). This might not be the correct place or method to do this, though; so if you can point me to a more proper solution, I'm all ears 🙂.

Thanks for the context. Maybe add a comment mentioning the same and we can improve later if needed.

ashutosh-narkar · 2024-08-20T19:17:49Z

debug/debugger.go

+	return nil
+}
+
+func (s *session) StepOver(threadID ThreadID) error {


Nit: StepOver, StepOut and StepIn have some duplicate code which we could move in a helper.

ashutosh-narkar · 2024-08-20T19:21:18Z

debug/debugger.go

+	}
+
+	if s.skipOp(e.Op) {
+		// FIXME: Should we only skip an event as long as we're within the same query scope?


What's the context here? Would be good to fix this.

This was an old comment we can remove. I think we're still gonna need to skip "undesired" operations if they're the next op when entering or leaving a new query level/context.

ashutosh-narkar · 2024-08-20T19:24:18Z

debug/debugger.go

+	return input, nil
+}
+
+type session struct {


Can this be accessed concurrently? I didn't see any locks so maybe not?

This is the primary entrypoint for e.g. implementations of the DAP protocol, so we can't rule out concurrent access completely. It should be benign to add a global mutex. I'll look into fixing that.

ashutosh-narkar · 2024-08-20T19:27:28Z

debug/debugger.go

+func (s *session) AddBreakpoint(loc location.Location) (Breakpoint, error) {
+	if s == nil {
+		return nil, fmt.Errorf("no active debug session")
+	}


Hmm interesting that SetBreakpoints clears existing breakpoints before adding new ones.

"Set" implies replacing what already exists, whereas add does not.
This was however a misread of the DAP protocol from me, where I originally thought setting breakpoints was a global operation, but it's actually a per-file operation. I solved this by instead making the user of the SDK make that distinction by using a combination of Breakpoints(), AddBreakpoint(), and RemoveBreakpoint().

Signed-off-by: Johan Fylling <[email protected]>

…key list Signed-off-by: Johan Fylling <[email protected]>

ashutosh-narkar · 2024-08-23T22:36:03Z

debug/thread.go

+			qid = e.QueryID
+		}
+
+		// FIXME: Compare against c.ParentID instead?


Aren't we using similar logic in stepOver? What's the concern here?

Yes, this isn't the only place where this could be something to consider. Likely, we wouldn't solve anything by doing this, though, as I think we could be "popped out" more than one query level at a time; making the parent ID irrelevant 🤔 .
I'll remove this comment.

ashutosh-narkar · 2024-08-23T22:41:58Z

debug/trace.go

+	"github.com/open-policy-agent/opa/topdown"
+)
+
+type stack interface {


Should this be called trace? Also we don't need to make this public?

There are two sides to this coin: from the perspective of topdown, this is a tracer; but from the view of the debugger, it behaves as a stack, so we're letting the naming reflect that perspective.

Currently, I don't think this needs to be public, but that could change in the future.

ashutosh-narkar · 2024-08-23T23:25:51Z

topdown/cache.go

@@ -19,7 +42,7 @@ type virtualCacheElem struct {
 	undefined bool
 }

-func newVirtualCache() *virtualCache {
+func NewVirtualCache() VirtualCache {


This is being used by the debugger implementation hence we're making this public. Couldn't we use an internal package for this and avoid exposing this?

Nvm. I just saw we're doing func (q *Query) WithVirtualCache(vc VirtualCache) *Query

ashutosh-narkar · 2024-08-23T23:31:57Z

util/test/tempus.go

@@ -11,6 +11,7 @@ import (
 	"time"
 )

+// FIXME: Abort long running tests


Anything you plan to do in this PR about this?

No. This was a thought that struck me when passing by. I'll create a ticket for tracking.

ashutosh-narkar

@johanfylling are you planning to add some documentation for this?

* Removing old FIXME Signed-off-by: Johan Fylling <[email protected]>

Signed-off-by: Johan Fylling <[email protected]>

netlify · 2024-08-28T12:41:41Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`02cc5e9`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/66cf419b12ef410008968a12
😎 Deploy Preview	https://deploy-preview-6877--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

johanfylling · 2024-08-28T12:42:56Z

debug/README.md

+    }
+}
+
+variables, err := session.Variables(localScope.VariablesReference())


This is a contender for some UX improvements.

Signed-off-by: Johan Fylling <[email protected]>

ashutosh-narkar

This is an excellent addition 👏

debug: Adding debugger

57021d6

Fixes: open-policy-agent#6876 Signed-off-by: Johan Fylling <[email protected]>

johanfylling mentioned this pull request Jul 18, 2024

Adding DAP implementation for debugging OPA StyraInc/regal#926

Merged

johanfylling added 2 commits July 18, 2024 14:52

Making linter happy

59d6e53

(mostly renaming) Signed-off-by: Johan Fylling <[email protected]>

Removing prints from tests

3f05fdf

Signed-off-by: Johan Fylling <[email protected]>

johanfylling force-pushed the debugger branch from 74e3970 to 3f05fdf Compare July 18, 2024 16:50

johanfylling added 3 commits July 18, 2024 19:21

Fixing race issue

40e9deb

Signed-off-by: Johan Fylling <[email protected]>

Adding Globals scope containing virtual cache entries

aef2d04

Signed-off-by: Johan Fylling <[email protected]>

Merge branch 'main' into debugger

a4ff525

charlieegan3 previously approved these changes Jul 24, 2024

View reviewed changes

johanfylling added 2 commits July 24, 2024 14:58

Merge branch 'main' into debugger

713e1c8

Updating source docs

6b6a708

Signed-off-by: Johan Fylling <[email protected]>

johanfylling dismissed charlieegan3’s stale review via 6b6a708 July 24, 2024 13:25

johanfylling requested a review from ashutosh-narkar July 24, 2024 13:25

ashutosh-narkar reviewed Jul 24, 2024

View reviewed changes

Fixing broken test

aaa8024

Signed-off-by: Johan Fylling <[email protected]>

johanfylling added 5 commits August 19, 2024 14:01

Merge branch 'main' into debugger

d21d8fe

Adding debugger breakpoint management functions

d0f6e61

Signed-off-by: Johan Fylling <[email protected]>

Merge branch 'main' into debugger

9339e4b

Adding Data variable scope

aee74fa

Signed-off-by: Johan Fylling <[email protected]>

Renaming Globals var scope to Virtual Cache

28ee6ff

Signed-off-by: Johan Fylling <[email protected]>

ashutosh-narkar reviewed Aug 20, 2024

View reviewed changes

johanfylling added 3 commits August 21, 2024 13:45

Fixing review issues

94d87a0

Signed-off-by: Johan Fylling <[email protected]>

Synchronizing public functions on session with mutex

f88a270

Signed-off-by: Johan Fylling <[email protected]>

Using ast.Ref.Append() when recursively constructing virtual cache …

9fb5601

…key list Signed-off-by: Johan Fylling <[email protected]>

ashutosh-narkar reviewed Aug 23, 2024

View reviewed changes

johanfylling added 3 commits August 26, 2024 21:04

Merge branch 'main' into debugger

76abcde

* Adding clarifying comment for store commit

43fac34

* Removing old FIXME Signed-off-by: Johan Fylling <[email protected]>

Adding debugger README

79d2e9f

Signed-off-by: Johan Fylling <[email protected]>

johanfylling commented Aug 28, 2024

View reviewed changes

Adding EXPERIMENTAL note to debug package

307f976

Signed-off-by: Johan Fylling <[email protected]>

johanfylling requested a review from ashutosh-narkar August 28, 2024 13:00

johanfylling added 2 commits August 28, 2024 17:26

Merge branch 'main' into debugger

02cc5e9

Merge branch 'main' into debugger

6f0d67a

ashutosh-narkar approved these changes Aug 28, 2024

View reviewed changes

johanfylling merged commit 3ac5104 into open-policy-agent:main Aug 28, 2024
28 checks passed

debug: Adding debugger #6877

debug: Adding debugger #6877

Conversation

johanfylling commented Jul 18, 2024

charlieegan3 left a comment

Choose a reason for hiding this comment

ashutosh-narkar left a comment

Choose a reason for hiding this comment

johanfylling commented Jul 24, 2024

ashutosh-narkar commented Jul 24, 2024

johanfylling commented Jul 25, 2024

anderseknert commented Aug 12, 2024

ashutosh-narkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanfylling Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutosh-narkar left a comment

Choose a reason for hiding this comment

netlify bot commented Aug 28, 2024 • edited Loading

✅ Deploy Preview for openpolicyagent ready!

Choose a reason for hiding this comment

ashutosh-narkar left a comment

Choose a reason for hiding this comment

johanfylling Aug 26, 2024 •

edited

Loading

netlify bot commented Aug 28, 2024 •

edited

Loading