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

contrib/gorm.io/gorm.v1: child database spans are not attached to the gorm parent span #849

Open
hatstand opened this issue Feb 18, 2021 · 13 comments · May be fixed by #2759
Open

contrib/gorm.io/gorm.v1: child database spans are not attached to the gorm parent span #849

hatstand opened this issue Feb 18, 2021 · 13 comments · May be fixed by #2759
Assignees
Labels
ack apm:ecosystem contrib/* related feature requests or bugs bug unintended behavior that has to be fixed

Comments

@hatstand
Copy link

(As discussed on #759)

If both gorm & the underlying *sql.DB connection are setup for tracing, spans for the underlying database connection queries are not attached to the gorm span created for the query.

Screenshot 2021-02-18 at 11 10 43

It's possible I've misconfigured something but it does seem like an actual issue from the above screenshot (the purple postgres.query span should be below the gorm.query span). I think this isn't an issue for jinzhu/gorm as there's no way to inject a traced sql.DB in the old API so you wouldn't have any sql spans at all.

For reproducing, we've got something like this setup:

import (
  sqltrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/database/sql"
  gormtrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/gorm.io/gorm.v1"
)

sqltrace.Register("postgres", &pq.Driver{})
db, _ := sqltrace.Open("postgres", "postgres://")
orm, _ := gormtrace.Open(postgres.New(postgres.Config{
  Conn: db,
})
@gbbr
Copy link
Contributor

gbbr commented Feb 18, 2021

Perfect. This is very helpful and paints a very clear picture. Thank you for taking the time to create this issue. Would you have any interest in doing the work? Otherwise we can schedule some work internally.

@hatstand
Copy link
Author

I might be able to find time to do this assuming it's as simple as creating the span in before instead of after.

@gbbr
Copy link
Contributor

gbbr commented Feb 18, 2021

I think it should be much easier than that. No need to move it to before. Wouldn't this work?

Replacing:

span, _ := tracer.StartSpanFromContext(ctx, operationName, opts...)

With:

var span ddtrace.Span
span, db.Statement.Context = tracer.StartSpanFromContext(ctx, operationName, opts...)

@gbbr
Copy link
Contributor

gbbr commented Feb 18, 2021

But I see what you mean. Never mind the above :) Perhaps we could store the span in the context, just like we do the start time now... I wonder if there's any risk to create a leak like that, considering someone never triggers the after...

@sjmtan
Copy link
Contributor

sjmtan commented Mar 2, 2021

I'm pretty sure this is impossible without rewriting Gorm? Since Gorm doesn't call ExecContext, or QueryContext, the context won't even be passed to link the two. I'm not sure how you do this. I imagine this is why Begin/Commit does show up tied to the parent spans - because Gorm calls BeginTx(ctx...)

Gorm uses these interfaces:

// SQLCommon is the minimal database connection functionality gorm requires.  Implemented by *sql.DB.
type SQLCommon interface {
	Exec(query string, args ...interface{}) (sql.Result, error)
	Prepare(query string) (*sql.Stmt, error)
	Query(query string, args ...interface{}) (*sql.Rows, error)
	QueryRow(query string, args ...interface{}) *sql.Row
}

type sqlDb interface {
	Begin() (*sql.Tx, error)
	BeginTx(ctx context.Context, opts *sql.TxOptions) (*sql.Tx, error)
}

type sqlTx interface {
	Commit() error
	Rollback() error
}

@sjmtan
Copy link
Contributor

sjmtan commented Mar 2, 2021

I guess that's what I see in my traces - only Begin/Commit show up associated, all others are their own root spans (which is really really annoying)

@knusbaum knusbaum added bug unintended behavior that has to be fixed apm:ecosystem contrib/* related feature requests or bugs labels May 26, 2021
@prasad-marne
Copy link
Contributor

Any progress or updates on this? i am facing same issue. If required i can help with the PR.

@knusbaum
Copy link
Contributor

Hi, @prasad-marne. We have not made any progress on this issue and any community help is welcome.

@admarko
Copy link

admarko commented Sep 29, 2021

Hey -- wondering if there's any update on this? Have other people found it a blocker from using? I want to upgrade from Gorm 1.0 to 2.0 but need to make sure the gorm2 wrapper for ddtrace is usable

@knusbaum knusbaum added the ack label Dec 16, 2021
@VinhVu95
Copy link

VinhVu95 commented Oct 26, 2022

Any updates on this issue? looks like it has not been addressed on the latest release of contrib/gorm.io/gorm.v1

@ervitis
Copy link

ervitis commented Mar 13, 2024

I don't know if it's related, but recently we updated gorm from v1 to v2 and used this example to help us to configure datadog with gorm #759. But the traces from the http and gorm database traces are not correlated and they are shown as separated traces as I show in the capture images.

Screenshot 2024-03-14 at 08 29 24

So, searching for possible workarounds, I found this issue in the gorm repository: go-gorm/gorm#1231

And use the function WithContext(ctx) to propagate the context. With this approach, the full trace is shown in DataDog panel.

Screenshot 2024-03-13 at 16 56 40

Could it be possible that DataDog cannot get the context?

@rarguelloF rarguelloF self-assigned this May 6, 2024
@p0937507934
Copy link

@ervitis
I'm currently encountering the same problem where I can't see the correlated traces between Gin and Gorm. They seem to be separated. However, the issue lies in the fact that I can see the Gorm tracing and the Gin tracing have their own root spans. On the other hand, the Gin Context doesn't seem to carry the right information. In my case, I passed the Gin Context from the handler to the repository layer where Gorm executed the query. Once I used db.WithContext(c.Request.Context) instead of the Gin Context, the correct tracing appeared in Datadog.

However, there's still one question remaining: if I want to use a goroutine to perform some asynchronous jobs, should I copy the Gin Context to maintain the information?
Does anyone know how to deal with this?

@System-Glitch
Copy link

We also have this issue (among others) with the Gorm tracer. We created a fork and are experimenting with potential fixes. So far we have made pretty good improvements.

main...onfocusio:dd-trace-go:chore/sc-13429/gorm-propagate-parent-span

If this solution sounds good to maintainers, maybe I could allocate some time updating the tests and opening a PR to get this merged into the official repo.

System-Glitch added a commit to onfocusio/dd-trace-go that referenced this issue Jun 25, 2024
- Start span in before callback instead of after to allow child context to be given to underlying DB driver
- Fix row and raw callbacks not being ordered correctly
- Don't generate spans for DryRun statements (e.g. when using a statement or subquery as var)

fixes DataDog#849
@System-Glitch System-Glitch linked a pull request Jun 25, 2024 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack apm:ecosystem contrib/* related feature requests or bugs bug unintended behavior that has to be fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.