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

The ctx position in return probe might be incorrect #1581

Closed
minimAluminiumalism opened this issue Jan 11, 2025 · 2 comments · Fixed by #1772
Closed

The ctx position in return probe might be incorrect #1581

minimAluminiumalism opened this issue Jan 11, 2025 · 2 comments · Fixed by #1772
Assignees
Labels
bug Something isn't working
Milestone

Comments

@minimAluminiumalism
Copy link
Contributor

This issue has already been mentioned in Slack, and it is posted here to track progress.

In the database/sql instrumentation, I found two pieces of code:
https://github.com/open-telemetry/opentelemetry-go-instrumentation/blob/e6f3f3cab8[…]2/internal/pkg/instrumentation/bpf/database/sql/bpf/probe.bpf.c

get_Go_context(ctx, 2, 0, true, &go_context);

https://github.com/open-telemetry/opentelemetry-go-instrumentation/blob/e6f3f3cab8[…]2/internal/pkg/instrumentation/bpf/database/sql/bpf/probe.bpf.c

UPROBE_RETURN(queryDC, struct sql_request_t, sql_events, events, 3, 0, true)

In the entry probe, the context_pos is 2, but in the return probe the context_pos is 3? I suppose the position in the return probe is also 2?

Although I believe there is an error of ctx pos, currently the functionality is still working properly.

@MrAlias
Copy link
Contributor

MrAlias commented Feb 7, 2025

This is working for versions of Go > 1.17 where the context is not used to get a consistent eBPF map key.

@MrAlias
Copy link
Contributor

MrAlias commented Feb 7, 2025

Verified this is incorrect.

Test

Update the database end-to-end test to use Go < 1.17 (see #1771 also)

diff --git a/internal/test/e2e/databasesql/Dockerfile b/internal/test/e2e/databasesql/Dockerfile
index a50dfc75..48551411 100644
--- a/internal/test/e2e/databasesql/Dockerfile
+++ b/internal/test/e2e/databasesql/Dockerfile
@@ -1,4 +1,4 @@
-FROM golang:1.23.6@sha256:927112936d6b496ed95f55f362cc09da6e3e624ef868814c56d55bd7323e0959
+FROM golang:1.16

 # pre-copy/cache go.mod for pre-downloading dependencies and only redownloading
 # them in subsequent builds if they change
diff --git a/internal/test/e2e/databasesql/go.mod b/internal/test/e2e/databasesql/go.mod
index bb73bb66..ca339c81 100644
--- a/internal/test/e2e/databasesql/go.mod
+++ b/internal/test/e2e/databasesql/go.mod
@@ -1,5 +1,5 @@
 module go.opentelemetry.io/auto/internal/test/e2e/databasesql

-go 1.22.0
+go 1.16

-require github.com/mattn/go-sqlite3 v1.14.24
+require github.com/mattn/go-sqlite3 v1.14.1
diff --git a/internal/test/e2e/databasesql/go.sum b/internal/test/e2e/databasesql/go.sum
index 9dcdc9b6..49dee20a 100644
--- a/internal/test/e2e/databasesql/go.sum
+++ b/internal/test/e2e/databasesql/go.sum
@@ -1,2 +1,11 @@
-github.com/mattn/go-sqlite3 v1.14.24 h1:tpSp2G2KyMnnQu99ngJ47EIkWVmliIizyZBfPrBWDRM=
-github.com/mattn/go-sqlite3 v1.14.24/go.mod h1:Uh1q+B4BYcTPb+yiD3kU8Ct7aC0hY9fxUwlHK0RXw+Y=
+github.com/PuerkitoBio/goquery v1.5.1/go.mod h1:GsLWisAFVj4WgDibEWF4pvYnkVQBpKBKeU+7zCJoLcc=
+github.com/andybalholm/cascadia v1.1.0/go.mod h1:GsXiBklL0woXo1j/WYWtSYYC4ouU9PqHO0sqidkEA4Y=
+github.com/mattn/go-sqlite3 v1.14.1 h1:AHx9Ra40wIzl+GelgX2X6AWxmT5tfxhI1PL0523HcSw=
+github.com/mattn/go-sqlite3 v1.14.1/go.mod h1:JIl7NbARA7phWnGvh0LKTyg7S9BA+6gx71ShQilpsus=
+golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
+golang.org/x/net v0.0.0-20180218175443-cbe0f9307d01/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
+golang.org/x/net v0.0.0-20200202094626-16171245cfb2/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
+golang.org/x/net v0.0.0-20200324143707-d3edc9973b7e/go.mod h1:qpuaurCH72eLCgpAm/N6yyVIVM9cpaDIP3A8BGJEC5A=
+golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
+golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
diff --git a/internal/test/e2e/databasesql/main.go b/internal/test/e2e/databasesql/main.go
index 67d83655..29471066 100644
--- a/internal/test/e2e/databasesql/main.go
+++ b/internal/test/e2e/databasesql/main.go
@@ -7,7 +7,7 @@ import (
        "database/sql"
        "fmt"
        "io"
-       "log/slog"
+       "log"
        "net/http"
        "os"
        "time"
@@ -80,11 +80,11 @@ func (s *Server) query(w http.ResponseWriter, req *http.Request, query string) {

        rows, err := conn.QueryContext(req.Context(), query)
        if err != nil {
-               slog.Error("query failed", "query", query, "error", err)
+               log.Println("query failed", "query", query, "error", err)
                return
        }

-       slog.Info("queryDB called", "query", query)
+       log.Println("queryDB called", "query", query)
        for rows.Next() {
                var id int
                var firstName string
@@ -125,7 +125,7 @@ func (s *Server) invalid(w http.ResponseWriter, req *http.Request) {

 func main() {
        port := fmt.Sprintf(":%d", 8080)
-       slog.Info("starting http server", "port", port)
+       log.Println("starting http server", "port", port)

        s := NewServer()

@@ -156,14 +156,14 @@ func main() {
        for _, t := range tests {
                resp, err := http.Get(t.url)
                if err != nil {
-                       slog.Error("failed GET", "error", err)
+                       log.Println("failed GET", "error", err)
                }
                if resp != nil {
                        body, err := io.ReadAll(resp.Body)
                        if err != nil {
-                               slog.Error("failed to read HTTP body", "error", err)
+                               log.Println("failed to read HTTP body", "error", err)
                        }
-                       slog.Info("request successful", "body", string(body[:]))
+                       log.Println("request successful", "body", string(body[:]))
                        _ = resp.Body.Close()
                }
        }

Run the test: make fixture-databasesql.

The output will be missing the database spans:

   @@ -49,165 +49,6 @@
          },
          "schemaUrl": "https://opentelemetry.io/schemas/1.26.0",
          "scopeSpans": [
   -        {
   -          "schemaUrl": "https://opentelemetry.io/schemas/1.26.0",
   -          "scope": {
   -            "name": "go.opentelemetry.io/auto/database/sql",
   -            "version": "v0.20.0"
   -          },
   -          "spans": [
   -            {
   -              "attributes": [
   -                {
   -                  "key": "db.query.text",
   -                  "value": {
   -                    "stringValue": "SELECT * FROM contacts"
   -                  }
   -                },
   -                {
   -                  "key": "db.operation.name",
   -                  "value": {
   -                    "stringValue": "SELECT"
   -                  }
   -                },
   -                {
   -                  "key": "db.collection.name",
   -                  "value": {
   -                    "stringValue": "contacts"
   -                  }
   -                }
   -              ],
   -              "flags": 256,
   -              "kind": 3,
   -              "name": "SELECT contacts",
   -              "parentSpanId": "xxxxx",
   -              "spanId": "xxxxx",
   -              "status": {},
   -              "traceId": "xxxxx"
   -            },
   -            {
   -              "attributes": [
   -                {
   -                  "key": "db.query.text",
   -                  "value": {
   -                    "stringValue": "INSERT INTO contacts (first_name) VALUES ('Mike')"
   -                  }
   -                },
   -                {
   -                  "key": "db.operation.name",
   -                  "value": {
   -                    "stringValue": "INSERT"
   -                  }
   -                },
   -                {
   -                  "key": "db.collection.name",
   -                  "value": {
   -                    "stringValue": "contacts"
   -                  }
   -                }
   -              ],
   -              "flags": 256,
   -              "kind": 3,
   -              "name": "INSERT contacts",
   -              "parentSpanId": "xxxxx",
   -              "spanId": "xxxxx",
   -              "status": {},
   -              "traceId": "xxxxx"
   -            },
   -            {
   -              "attributes": [
   -                {
   -                  "key": "db.query.text",
   -                  "value": {
   -                    "stringValue": "UPDATE contacts SET last_name = 'Santa' WHERE first_name = 'Mike'"
   -                  }
   -                },
   -                {
   -                  "key": "db.operation.name",
   -                  "value": {
   -                    "stringValue": "UPDATE"
   -                  }
   -                },
   -                {
   -                  "key": "db.collection.name",
   -                  "value": {
   -                    "stringValue": "contacts"
   -                  }
   -                }
   -              ],
   -              "flags": 256,
   -              "kind": 3,
   -              "name": "UPDATE contacts",
   -              "parentSpanId": "xxxxx",
   -              "spanId": "xxxxx",
   -              "status": {},
   -              "traceId": "xxxxx"
   -            },
   -            {
   -              "attributes": [
   -                {
   -                  "key": "db.query.text",
   -                  "value": {
   -                    "stringValue": "DELETE FROM contacts WHERE first_name = 'Mike'"
   -                  }
   -                },
   -                {
   -                  "key": "db.operation.name",
   -                  "value": {
   -                    "stringValue": "DELETE"
   -                  }
   -                },
   -                {
   -                  "key": "db.collection.name",
   -                  "value": {
   -                    "stringValue": "contacts"
   -                  }
   -                }
   -              ],
   -              "flags": 256,
   -              "kind": 3,
   -              "name": "DELETE contacts",
   -              "parentSpanId": "xxxxx",
   -              "spanId": "xxxxx",
   -              "status": {},
   -              "traceId": "xxxxx"
   -            },
   -            {
   -              "attributes": [
   -                {
   -                  "key": "db.query.text",
   -                  "value": {
   -                    "stringValue": "DROP TABLE contacts"
   -                  }
   -                }
   -              ],
   -              "flags": 256,
   -              "kind": 3,
   -              "name": "DB",
   -              "parentSpanId": "xxxxx",
   -              "spanId": "xxxxx",
   -              "status": {},
   -              "traceId": "xxxxx"
   -            },
   -            {
   -              "attributes": [
   -                {
   -                  "key": "db.query.text",
   -                  "value": {
   -                    "stringValue": "syntax error"
   -                  }
   -                }
   -              ],
   -              "flags": 256,
   -              "kind": 3,
   -              "name": "DB",
   -              "parentSpanId": "xxxxx",
   -              "spanId": "xxxxx",
   -              "status": {},
   -              "traceId": "xxxxx"
   -            }
   -          ]
   -        },

Change the arg position:

diff --git a/internal/pkg/instrumentation/bpf/database/sql/bpf/probe.bpf.c b/internal/pkg/instrumentation/bpf/database/sql/bpf/probe.bpf.c
index 9ef6086d..a814e1d4 100644
--- a/internal/pkg/instrumentation/bpf/database/sql/bpf/probe.bpf.c
+++ b/internal/pkg/instrumentation/bpf/database/sql/bpf/probe.bpf.c
@@ -69,7 +69,7 @@ int uprobe_queryDC(struct pt_regs *ctx) {

 // This instrumentation attaches uprobe to the following function:
 // func (db *DB) queryDC(ctx, txctx context.Context, dc *driverConn, releaseConn func(error), query string, args []any)
-UPROBE_RETURN(queryDC, struct sql_request_t, sql_events, events, 3, 0, true)
+UPROBE_RETURN(queryDC, struct sql_request_t, sql_events, events, 2, 0, true)

 // This instrumentation attaches uprobe to the following function:
 // func (db *DB) execDC(ctx context.Context, dc *driverConn, release func(error), query string, args []any)

Re-run the end-to-end test and the expected spans are now exported.

@MrAlias MrAlias added this to the v0.21.0 milestone Feb 7, 2025
@MrAlias MrAlias self-assigned this Feb 7, 2025
@MrAlias MrAlias added the bug Something isn't working label Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants