fix: import path may have alias name (#172)

* fix: import path may have alias name

* fix once again

* better

* preclean should not remove deps

* better logo

* use %q format specifier

* fine grind log

Author: y1yang0

README.md

@@ -6,13 +6,13 @@
 
 This project provides an automatic solution for Golang applications that want to
 leverage OpenTelemetry to enable effective observability. No code changes are
-required in the target application, and the instrumentation is done at compile
-time. Simply replace `go build` with `otelbuild` to get started.
+required in the target application, the instrumentation is done at compile
+time. Simply replacing `go build` with `otelbuild` to get started :rocket:
 
 # Installation
 
 ### Install via Bash
-For **Linux and MacOS** users, install the tool by running the following command :rocket:
+For **Linux and MacOS** users, install the tool by running the following command
 ```bash
 $ sudo curl -fsSL https://cdn.jsdelivr.net/gh/alibaba/opentelemetry-go-auto-instrumentation@main/install.sh | sudo bash
 ```

docs/anim-logo.svg

@@ -12,6 +12,12 @@
         "OnEnter": "onEnterTestSkip",
         "Path": "github.com/alibaba/opentelemetry-go-auto-instrumentation/pkg/rules/test/error2"
     },
+    {
+        "ImportPath": "errors",
+        "Function": "New",
+        "OnEnter": "onEnterErrorsNewAlias",
+        "Path": "github.com/alibaba/opentelemetry-go-auto-instrumentation/pkg/rules/test/error3"
+    },
     {
         "ImportPath": "errorstest/auxiliary",
         "Function": "TestSkip",

pkg/data/test_error.json

@@ -0,0 +1,26 @@
+// Copyright (c) 2024 Alibaba Group Holding Ltd.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package error2
+
+import (
+	erralias "errors"
+
+	"github.com/alibaba/opentelemetry-go-auto-instrumentation/pkg/api"
+)
+
+func onEnterErrorsNewAlias(call api.CallContext, text string) {
+	// Check if alias name confuses compilation
+	_ = erralias.ErrUnsupported
+}

pkg/rules/test/error3/hook.go

@@ -33,7 +33,7 @@ func replaceImport(importPath string, code string) string {
 	return code
 }
 
-func (dp *DepProcessor) replaceOtelImports(compileCmds []string) error {
+func (dp *DepProcessor) replaceOtelImports() error {
 	moduleName, err := dp.getImportPathOf(OtelPkgDepsDir)
 	if err != nil {
 		return fmt.Errorf("failed to get import path of otel_pkg: %w", err)
@@ -42,7 +42,7 @@ func (dp *DepProcessor) replaceOtelImports(compileCmds []string) error {
 	for _, dep := range []string{OtelRules, OtelPkgDepsDir} {
 		files, err := util.ListFiles(dep)
 		if err != nil {
-			return fmt.Errorf("failed to list files during replacement: %w", err)
+			return fmt.Errorf("failed to list files: %w", err)
 		}
 		for _, file := range files {
 			// Skip non-go files as no imports within them

tool/preprocess/pkgdep.go

@@ -55,7 +55,7 @@ const (
 	DryRunLog        = "dry_run.log"
 	StdRulesPrefix   = "github.com/alibaba/opentelemetry-go-auto-instrumentation/pkg/"
 	StdRulesPath     = "github.com/alibaba/opentelemetry-go-auto-instrumentation/pkg/rules"
-	apiImport        = "github.com/alibaba/opentelemetry-go-auto-instrumentation/pkg/api"
+	ApiPath          = "github.com/alibaba/opentelemetry-go-auto-instrumentation/pkg/api"
 )
 
 // @@ Change should sync with trampoline template
@@ -312,7 +312,9 @@ func (dp *DepProcessor) addExplicitImport(importPaths ...string) (err error) {
 		// Prepend import path to the file
 		for _, importPath := range importPaths {
 			shared.AddImportForcely(astRoot, importPath)
-			log.Printf("Add %s import to %v", importPath, file)
+			if shared.Verbose {
+				log.Printf("Add %s import to %v", importPath, file)
+			}
 		}
 		addImport = true
 
@@ -410,23 +412,12 @@ func (dp *DepProcessor) preclean() {
 		if astRoot == nil {
 			continue
 		}
-		if shared.RemoveImport(astRoot, ruleImport) {
+		if shared.RemoveImport(astRoot, ruleImport) != nil {
 			if shared.Verbose {
 				log.Printf("Remove obsolete import %v from %v",
 					ruleImport, file)
 			}
 		}
-		for _, dep := range fixedDeps {
-			if !dep.addImport {
-				continue
-			}
-			if shared.RemoveImport(astRoot, dep.dep) {
-				if shared.Verbose {
-					log.Printf("Remove obsolete import %v from %v",
-						dep, file)
-				}
-			}
-		}
 		_, err := shared.WriteAstToFile(astRoot, file)
 		if err != nil {
 			log.Printf("Failed to write ast to %v: %v", file, err)
@@ -552,7 +543,9 @@ func (dp *DepProcessor) pinDepVersion() error {
 	for _, dep := range fixedDeps {
 		p := dep.dep
 		v := dep.version
-		log.Printf("Pin dependency version %v@%v", p, v)
+		if shared.Verbose {
+			log.Printf("Pin dependency version %v@%v", p, v)
+		}
 		err := fetchDep(p + "@" + v)
 		if err != nil {
 			if dep.fallible {
@@ -664,7 +657,7 @@ func (dp *DepProcessor) setupDeps() error {
 		return fmt.Errorf("failed to setup dependencies: %w", err)
 	}
 
-	err = dp.replaceOtelImports(compileCmds)
+	err = dp.replaceOtelImports()
 	if err != nil {
 		return fmt.Errorf("failed to replace otel imports: %w", err)
 	}

tool/preprocess/preprocess.go

@@ -16,6 +16,7 @@ package preprocess
 
 import (
 	"fmt"
+	"go/token"
 	"log"
 	"os"
 	"path/filepath"
@@ -101,23 +102,24 @@ func (dp *DepProcessor) copyRules(target string) (err error) {
 		}
 	}
 
-	// for path, bundle := range uniqueResources {
-	// 	dir := res2Dir[path]
-	// 	ruleDir := filepath.Join(target, dir)
-	// 	err = os.MkdirAll(ruleDir, 0777)
-	// 	if err != nil {
-	// 		return fmt.Errorf("failed to create directory %v: %w", ruleDir, err)
-	// 	}
-	// 	ruleFile := filepath.Join(ruleDir, filepath.Base(path))
-	// 	err = dp.copyRule(path, ruleFile, bundle)
-	// 	if err != nil {
-	// 		return fmt.Errorf("failed to copy rule %v: %w", path, err)
-	// 	}
-	// }
 	return nil
 }
 
-func renameCallContext(astRoot *dst.File, pkgName string) {
+func renameCallContext(astRoot *dst.File, bundle *resource.RuleBundle) {
+	pkgName := bundle.PackageName
+	// Find out if the target import path is aliased to another name
+	// if so, we need to rename api.CallContext to the alias name
+	// instead of the package name
+	for _, spec := range astRoot.Imports {
+		// Same import path and alias name is not null?
+		// One exception is the alias name is "_", we should ignore it
+		if shared.IsStringLit(spec.Path, bundle.ImportPath) &&
+			spec.Name != nil &&
+			spec.Name.Name != shared.IdentIgnore {
+			pkgName = spec.Name.Name
+			break
+		}
+	}
 	for _, decl := range astRoot.Decls {
 		if f, ok := decl.(*dst.FuncDecl); ok {
 			params := f.Type.Params.List
@@ -165,6 +167,45 @@ func makeHookPublic(astRoot *dst.File, bundle *resource.RuleBundle) {
 	}
 }
 
+func renameImport(root *dst.File, oldPath, newPath string) bool {
+	// Find out if the old import and replace it with new one. Why we dont
+	// remove old import and add new one? Because we are not sure if the
+	// new import will be used, it's a compilation error if we import it
+	// but never use it.
+	for _, decl := range root.Decls {
+		if genDecl, ok := decl.(*dst.GenDecl); ok &&
+			genDecl.Tok == token.IMPORT {
+			for _, spec := range genDecl.Specs {
+				if importSpec, ok := spec.(*dst.ImportSpec); ok {
+					if importSpec.Path.Value == fmt.Sprintf("%q", oldPath) {
+						// In case the new import is already present, try to
+						// remove it first
+						oldSpec := shared.RemoveImport(root, newPath)
+						// Replace old with new one
+						importSpec.Path.Value = fmt.Sprintf("%q", newPath)
+						// Respect alias name of old import, if any
+						if oldSpec != nil {
+							importSpec.Name = oldSpec.Name
+
+							// Unless the alias name is "_", we should keep it
+							// For "_" alias, we should add additional normal
+							// variant for CallContext usage, i.e. keep both
+							// imports, one for existing usages, one for
+							// CallContext usage
+							if oldSpec.Name != nil &&
+								oldSpec.Name.Name == shared.IdentIgnore {
+								shared.AddImport(root, newPath)
+							}
+						}
+						return true
+					}
+				}
+			}
+		}
+	}
+	return false
+}
+
 func (dp *DepProcessor) copyRule(path, target string,
 	bundle *resource.RuleBundle) error {
 	text, err := util.ReadFile(path)
@@ -180,13 +221,13 @@ func (dp *DepProcessor) copyRule(path, target string,
 	astRoot.Name.Name = filepath.Base(filepath.Dir(target))
 
 	// Rename api.CallContext to correct package name if present
-	renameCallContext(astRoot, bundle.PackageName)
+	renameCallContext(astRoot, bundle)
 
 	// Make hook functions public
 	makeHookPublic(astRoot, bundle)
 
-	// Remove "api" import if present
-	shared.ReplaceImport(astRoot, apiImport, bundle.ImportPath)
+	// Rename "api" import to the correct package prefix
+	renameImport(astRoot, ApiPath, bundle.ImportPath)
 
 	// Copy used rule into project
 	_, err = shared.WriteAstToFile(astRoot, target)
@@ -287,7 +328,7 @@ func (dp *DepProcessor) initRules(pkgName, target string) (err error) {
 		imports[OtelGetStackImportPath] = OtelGetStackAliasPkg
 	}
 	for k, v := range imports {
-		c += fmt.Sprintf("import %s \"%s\"\n", v, k)
+		c += fmt.Sprintf("import %s %q\n", v, k)
 	}
 
 	// Assignments

tool/preprocess/setup.go

@@ -68,6 +68,13 @@ func StringLit(value string) *dst.BasicLit {
 	}
 }
 
+func IsStringLit(expr dst.Expr, val string) bool {
+	lit, ok := expr.(*dst.BasicLit)
+	return ok &&
+		lit.Kind == token.STRING &&
+		lit.Value == fmt.Sprintf("%q", val)
+}
+
 func IntLit(value int) *dst.BasicLit {
 	return &dst.BasicLit{
 		Kind:  token.INT,
@@ -235,63 +242,52 @@ func AddStructField(decl dst.Decl, name string, typ string) {
 	st.Fields.List = append(st.Fields.List, fd)
 }
 
-func AddImportForcely(root *dst.File, path string) {
-	importStmt := &dst.GenDecl{
-		Tok: token.IMPORT,
-		Specs: []dst.Spec{
-			&dst.ImportSpec{
-				Name: &dst.Ident{Name: IdentIgnore},
-				Path: &dst.BasicLit{
-					Kind:  token.STRING,
-					Value: fmt.Sprintf("\"%s\"", path),
-				},
-			},
+func addImport(root *dst.File, path string, force bool) {
+	spec := &dst.ImportSpec{
+		Path: &dst.BasicLit{
+			Kind:  token.STRING,
+			Value: fmt.Sprintf("%q", path),
 		},
 	}
+	if force {
+		spec.Name = &dst.Ident{Name: IdentIgnore}
+	}
+	importStmt := &dst.GenDecl{
+		Tok:   token.IMPORT,
+		Specs: []dst.Spec{spec},
+	}
 	root.Decls = append([]dst.Decl{importStmt}, root.Decls...)
 }
 
-func RemoveImport(root *dst.File, path string) bool {
+func AddImportForcely(root *dst.File, path string) {
+	addImport(root, path, true)
+}
+
+func AddImport(root *dst.File, path string) {
+	addImport(root, path, false)
+}
+
+func RemoveImport(root *dst.File, path string) *dst.ImportSpec {
 	for j, decl := range root.Decls {
 		if genDecl, ok := decl.(*dst.GenDecl); ok &&
 			genDecl.Tok == token.IMPORT {
 			for i, spec := range genDecl.Specs {
 				if importSpec, ok := spec.(*dst.ImportSpec); ok {
-					if importSpec.Path.Value == fmt.Sprintf("\"%s\"", path) {
+					if importSpec.Path.Value == fmt.Sprintf("%q", path) {
 						genDecl.Specs =
 							append(genDecl.Specs[:i],
 								genDecl.Specs[i+1:]...)
 						if len(genDecl.Specs) == 0 {
 							root.Decls =
 								append(root.Decls[:j], root.Decls[j+1:]...)
 						}
-						return true
+						return importSpec
 					}
 				}
 			}
 		}
 	}
-	return false
-}
-
-func ReplaceImport(root *dst.File, oldPath, newPath string) bool {
-	for _, decl := range root.Decls {
-		if genDecl, ok := decl.(*dst.GenDecl); ok &&
-			genDecl.Tok == token.IMPORT {
-			for _, spec := range genDecl.Specs {
-				if importSpec, ok := spec.(*dst.ImportSpec); ok {
-					if importSpec.Path.Value == fmt.Sprintf("\"%s\"", oldPath) {
-						// In case the import is already present, try to remove
-						// it first
-						RemoveImport(root, newPath)
-						importSpec.Path.Value = fmt.Sprintf("\"%s\"", newPath)
-						return true
-					}
-				}
-			}
-		}
-	}
-	return false
+	return nil
 }
 
 func NewVarDecl(name string, paramTypes *dst.FieldList) *dst.GenDecl {