Read module dependencies from debug.BuildInfo instead of go.mod #199

wingyplus · 2020-04-14T17:26:38Z

This changes getting modules from runtime/debug instead of getting it from go.mod or vendor/modules.txt.

Fixes #184

wingyplus · 2020-04-14T17:28:05Z

From the image above, the sentry module that's mark as version 0.0.0 is replace module. I'm not sure how do we display this kind of module in sentry. The idea that I think will be add remark after version such as v0.0.0 (replaced by ).

rhcarvalho · 2020-04-14T19:50:21Z

@wingyplus thanks for your contribution!

Could you please update go.mod to go 1.12 -- debug.ReadBuildInfo requires 1.12 and that's the minimum version we support today anyway.

rhcarvalho

Thanks for contributing ❤️ !
Good start, I have a feel suggestions below.

I noticed this integration went in and never got proper tests. If that's something you'd like to contribute too, would be very welcome. Specially now, testing it got much simpler since we don't depend on parsing external files.

The basic test cases would be around the kinds of output debug.ReadBuildInfo() can return: no replacements, replacements with mod+version, absolute path, relative path.

rhcarvalho · 2020-04-14T19:30:32Z

integrations.go

-		if fileExists("vendor/modules.txt") {
-			return getModulesFromVendorTxt()
-		}
-
 		if fileExists("vendor/vendor.json") {


This is AFAICT used by the old govendor tool.
There is no reason for us to keep supporting that, govendor being only one among other once popular tools from the past (like dep, godep, glide, ...).

Go Modules are the present and the future, we can concentrate on that use case. Prior to Modules, there are so many ways to track dependencies and versions that we can't reasonably support everything. Users can still roll their own integration to capture this info if needed and not using Modules.

Would you like to have the pleasure to delete this portion of the code too?

In doing so, you'll likely realize there is no need for 3 functions: extractModules, getModules and getModulesFromBuildInfo -- we can move everything into one.

rhcarvalho · 2020-04-14T19:59:22Z

integrations.go

+func getModulesFromBuildInfo() (map[string]string, error) {
+	info, ok := debug.ReadBuildInfo()
+	if !ok {
+		return nil, fmt.Errorf("cannot dependencies from debug.BuildInfo")


As we can see from #197, returning an error here will only confuse users.

The case when ok = False is when the main package is built with GO111MODULE=off, so there's nothing the integration can do anyway.

We can log a message to help with debugging "why package/module information is not appearing in Sentry?" but the message should not be about a "failure", but rather say this integration is disabled when build is not using Go Modules.

We can use a similar message as documented in https://golang.org/pkg/runtime/debug/#ReadBuildInfo:

"The Modules integration is not available in binaries built without module support."

rhcarvalho · 2020-04-14T20:18:11Z

integrations.go

+	modules := make(map[string]string)
+	for _, dep := range info.Deps {
+		modules[dep.Path] = dep.Version


Needs to include info.Main too 😉

As you well noted in a comment, we also need to handle replacements. The Sentry server only takes a map[string]string for package/module name => version, no concept of replacements. While we could look into adding replacements server-side and in the web interface, I think we can well start with just reporting replacements within the current server constraints.

There are replacements pointing to a module+version, and replacements pointing to absolute local path and relative local path, see https://github.com/golang/go/wiki/Modules#when-should-i-use-the-replace-directive.

We could use a syntax similar to how go list -m all prints replacements:

module/name v0.0.0 => replacement

map[string]string{"module/name": "v0.0.0 => replacement"}

What do you think?

I think it sound good to me for doing this.

wingyplus · 2020-04-15T07:58:35Z

@rhcarvalho Thanks for your review. Main issue was adressed.

The basic test cases would be around the kinds of output debug.ReadBuildInfo() can return: no replacements, replacements with mod+version, absolute path, relative path.

Integration tests would a bit hard because debug.ReadBuildInfo didn't inject modules in test mode (see golang/go#33976). Mocking debug.ReadBuildInfo might be a bit easier. The illustrate that I think is:

var readBuildInfo = debug.ReadBuildInfo // this way we can inject a function that returns a modules list. 

func extractModules(){
  info, err := readBuildInfo()
  // ....
}

Do you have any suggestion?

rhcarvalho

Mocking debug.ReadBuildInfo might be a bit easier. The illustrate that I think is:
var readBuildInfo = debug.ReadBuildInfo // this way we can inject a function that returns a modules list. 

func extractModules(){
  info, err := readBuildInfo()
  // ....
}
Do you have any suggestion?

What you wrote works. There is one drawback that is global state, and tests could not run in parallel if we ever wanted to.

I do have a suggestion for an alternative design. Here's a bit of a skeleton:

func extractModules(*debug.BuildInfo) map[string]string {
    // ...
}

// ...
info, ok := debug.ReadBuildInfo()
// ...
_ = extractModules(info)

func Test...(t *testing.T) {
    tests := []...
    // ...

    for _, tt := range tests {
        tt := tt
        // ...
        t.Run( //...
           got := extractModules(tt.info)
           if diff := cmp.Diff(tt.want, got); diff != "" {
                // ...
           }
        })
    }
}

With this we can have several test cases that each define the input as a *debug.BuildInfo and the corresponding expected map[string]string. This way we are back to simple input-output comparisons and no need for any mocks or other supporting code.

What do you think?

rhcarvalho · 2020-04-15T12:43:12Z

integrations.go

+	for _, dep := range info.Deps {
+		ver := dep.Version
+		if dep.Replace != nil {
+			ver += fmt.Sprintf(" => %s %s", dep.Replace.Path, dep.Version)


When the replacement is coming from a local path, there is no version (empty string).

This is my mistake. It should be dep.Replace.Version not dep.Version.

@rhcarvalho Fixed

wingyplus · 2020-04-15T14:05:42Z

I do have a suggestion for an alternative design. Here's a bit of a skeleton:

func extractModules(*debug.BuildInfo) map[string]string {
// ...
}

// ...
info, ok := debug.ReadBuildInfo()
// ...
_ = extractModules(info)

func Test...(t *testing.T) {
tests := []...
// ...
for _, tt := range tests {
    tt := tt
    // ...
    t.Run( //...
       got := extractModules(tt.info)
       if diff := cmp.Diff(tt.want, got); diff != "" {
            // ...
       }
    })
}
}

With this we can have several test cases that each define the input as a *debug.BuildInfo and the corresponding expected map[string]string. This way we are back to simple input-output comparisons and no need for any mocks or other supporting code.

What do you think?

@rhcarvalho That's great idea! Let's try.

Fixes getsentry#184

The helper assertEqual takes (got, want).

When a test fails, cmp.Diff produces output which makes it easier to spot the difference, what went wrong.

Because now we're printing a fixed string, without string formatting.

rhcarvalho

@wingyplus awesome PR. Thank you!! 🥳

Going to release this into 0.6.0.

rhcarvalho · 2020-04-17T10:13:34Z

integrations_test.go

+			info: &debug.BuildInfo{
+				Main: debug.Module{
+					Path:    "my/module",
+					Version: "(devel)",


I just realized that at the moment the Version of the main module (in fact, the module that contains package main) is always "(devel)". That is unfortunate, but at least the module name will be in Sentry. Proper version can be added using the Releases feature.

golang/go#29228

rhcarvalho · 2020-04-17T11:43:18Z

integrations_test.go

+			if diff := cmp.Diff(tt.want, got); diff != "" {
+				t.Errorf("modules info mismatch (-want +got):\n%s", diff)
+			}


I took the liberty to change this to use cmp.Diff to make it easier to spot problems when tests fail.

@rhcarvalho I think we can turn assertEquals to uses cmp.Diff to display the diff when it's not equal. What do you think?

I don't see the point, as it just limits the usability. Personally I prefer calling cmp.Diff directly, it is more clear and flexible.

assertEquals always calls t.Fatal*, even when t.Error* would be more appropriate. Moving cmp.Diff into assertEquals would also limit control of comparison options.

Ah, I see. Thanks for your point.

wingyplus · 2020-04-17T14:02:13Z

@rhcarvalho thanks!! 😃

rhcarvalho suggested changes Apr 14, 2020

View reviewed changes

rhcarvalho linked an issue Apr 14, 2020 that may be closed by this pull request

[Sentry] ModuleIntegration wasn't able to extract modules: module integration failed #197

Closed

wingyplus force-pushed the iss-184 branch from d331cc6 to a304638 Compare April 15, 2020 04:02

rhcarvalho reviewed Apr 15, 2020

View reviewed changes

rhcarvalho added this to Review in progress in Sentry SDK for Go Apr 15, 2020

wingyplus requested a review from rhcarvalho April 15, 2020 14:47

rhcarvalho added this to the v0.6.0 milestone Apr 17, 2020

wingyplus and others added 11 commits April 17, 2020 10:19

Read module dependencies from debug.BuildInfo instead of go.mod

ef35bee

Fixes getsentry#184

Require go v1.12

20f9359

Remove another version of parse module, annotate replacement module

b86dee2

Add main module information to modules integration

3b8b7dd

Update log message

10135ac

Change extractModules to accept debug.BuildInfo and add tests

a7740e4

Use dep.Replace.Version, add more tests

72e3d05

Use sub test

a168844

Swap arguments to assertEqual

baaaf74

The helper assertEqual takes (got, want).

Use cmp.Diff instead of assertEqual

9aedba0

When a test fails, cmp.Diff produces output which makes it easier to spot the difference, what went wrong.

Use Print instead of Printf

b3548d6

Because now we're printing a fixed string, without string formatting.

rhcarvalho force-pushed the iss-184 branch from f42d38b to b3548d6 Compare April 17, 2020 11:41

rhcarvalho approved these changes Apr 17, 2020

View reviewed changes

rhcarvalho merged commit 5747ddf into getsentry:master Apr 17, 2020

Sentry SDK for Go automation moved this from Review in progress to Done Apr 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read module dependencies from debug.BuildInfo instead of go.mod #199

Read module dependencies from debug.BuildInfo instead of go.mod #199

wingyplus commented Apr 14, 2020

wingyplus commented Apr 14, 2020 •

edited

rhcarvalho commented Apr 14, 2020

rhcarvalho left a comment

rhcarvalho Apr 14, 2020

rhcarvalho Apr 14, 2020

rhcarvalho Apr 14, 2020

wingyplus Apr 15, 2020

wingyplus Apr 15, 2020

wingyplus commented Apr 15, 2020 •

edited

rhcarvalho left a comment

rhcarvalho Apr 15, 2020

wingyplus Apr 15, 2020

wingyplus Apr 15, 2020

wingyplus commented Apr 15, 2020

rhcarvalho left a comment

rhcarvalho Apr 17, 2020

rhcarvalho Apr 17, 2020

wingyplus Apr 20, 2020

rhcarvalho Apr 20, 2020

wingyplus Apr 20, 2020

wingyplus commented Apr 17, 2020

Read module dependencies from debug.BuildInfo instead of go.mod #199

Read module dependencies from debug.BuildInfo instead of go.mod #199

Conversation

wingyplus commented Apr 14, 2020

wingyplus commented Apr 14, 2020 • edited

rhcarvalho commented Apr 14, 2020

rhcarvalho 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

wingyplus commented Apr 15, 2020 • edited

rhcarvalho 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

wingyplus commented Apr 15, 2020

rhcarvalho 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

wingyplus commented Apr 17, 2020

wingyplus commented Apr 14, 2020 •

edited

wingyplus commented Apr 15, 2020 •

edited