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

Add DB sql parsing to processor #2022

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

damemi
Copy link
Contributor

@damemi damemi commented Dec 18, 2024

Adds https://github.com/odigos-io/enterprise-go-instrumentation/pull/896 to the odigossqldboperationprocessor. This gives us coverage for all languages and shifts the parsing out of the agent

@damemi damemi requested review from RonFed and tamirdavid1 December 18, 2024 16:00
@damemi damemi force-pushed the db-sql-attrs branch 2 times, most recently from 203a29d to b079e98 Compare December 18, 2024 16:49
Comment on lines 5 to 7
go 1.23.3

toolchain go1.23.4
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RonFed fyi this happened switching to the vitess library:

$ make go-mod-tidy
go: finding module for package vitess.io/vitess/go/vt/sqlparser
go: downloading vitess.io/vitess v0.21.1
go: toolchain upgrade needed to resolve vitess.io/vitess/go/vt/sqlparser
go: vitess.io/[email protected] requires go >= 1.23.3; switching to go1.23.4
go: downloading go1.23.4 (darwin/arm64)
go: finding module for package vitess.io/vitess/go/vt/sqlparser
go: found vitess.io/vitess/go/vt/sqlparser in vitess.io/vitess v0.21.1

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove the toolchain line, run go mod tidy again and it won't require the toolchain part but I'm not sure.

Copy link
Collaborator

@RonFed RonFed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some small comments.
I think we should do a load test with DB spans to make sure the CPU overhead of this processor is not too large.


// If we have a new span name, use that (but only if the current span name is
// our default "DB" auto-instrumentation name).
if spanName != "" && span.Name() == "DB" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DB name is specific to go auto instrumentation.
This creates some coupling. However, I don't have a suggestion for a better solution here since overriding the span name might not be desirable in a lot of cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly my thought, I'm assuming the other languages are able to follow the span name semconv easier? It would be nice if our default name from auto-instrumentation was something more indicative

@damemi damemi force-pushed the db-sql-attrs branch 2 times, most recently from fc672de to 5241393 Compare December 19, 2024 14:56
@tamirdavid1
Copy link
Collaborator

Mike can u check the how it is effect the cpu on the gateway if exists? we can do it together of course

@damemi
Copy link
Contributor Author

damemi commented Dec 19, 2024

Seems like maybe there is a goroutine leak in the vitess library? Based on this failure that only showed up once I switched to it.

goleak: Errors on successful test run: found unexpected goroutines:
[Goroutine 19 in state select, with github.com/golang/glog.(*fileSink).flushDaemon on top of the stack:
github.com/golang/glog.(*fileSink).flushDaemon(0x19af1f8)
	/home/runner/go/pkg/mod/github.com/golang/[email protected]/glog_file.go:349 +0xb4
created by github.com/golang/glog.init.1 in goroutine 1
	/home/runner/go/pkg/mod/github.com/golang/[email protected]/glog_file.go:164 +0x126
]
FAIL	odigossqldboperationprocessor	0.452s
FAIL
make: *** [Makefile:62: test/./processors/odigossqldboperationprocessor/go.mod] Error 1

Or I'm not closing something properly with that library. I'll try to resolve that, then do the performance checks that @tamirdavid1 mentioned

@tamirdavid1
Copy link
Collaborator

Seems like maybe there is a goroutine leak in the vitess library? Based on this failure that only showed up once I switched to it.

goleak: Errors on successful test run: found unexpected goroutines:
[Goroutine 19 in state select, with github.com/golang/glog.(*fileSink).flushDaemon on top of the stack:
github.com/golang/glog.(*fileSink).flushDaemon(0x19af1f8)
	/home/runner/go/pkg/mod/github.com/golang/[email protected]/glog_file.go:349 +0xb4
created by github.com/golang/glog.init.1 in goroutine 1
	/home/runner/go/pkg/mod/github.com/golang/[email protected]/glog_file.go:164 +0x126
]
FAIL	odigossqldboperationprocessor	0.452s
FAIL
make: *** [Makefile:62: test/./processors/odigossqldboperationprocessor/go.mod] Error 1

Or I'm not closing something properly with that library. I'll try to resolve that, then do the performance checks that @tamirdavid1 mentioned

Here to help whatever needed

@damemi
Copy link
Contributor Author

damemi commented Dec 23, 2024

@tamirdavid1 @RonFed the memory leak looked like it was from glog, so I decided to look at #2038 to see if it could have just been a bug in the otel version pulled in by updating to vitess.

But, the more I think about it, I don't think we should use vitess. It's been problematic to get to work for several reasons:

  • It links some Go runtime functions. This means we'll always have to be on the same Go version as vitess, which is a weird spot to tie ourselves to a dependency.
    • Technically this isn't done by the sqlparser package (it's vitess itself) but because that package isn't its own module we end up pulling in more of vitess than we need.
  • This leads to my second point, that the vitess sql package doesn't seem like it's meant to be used as a standalone library. That means we don't get any compatibility or support guarantees and we're taking on a lot of risks by using it

I reverted the change to vitess, and I'm working on getting some profiling data now. @tamirdavid1 would you be able to get the profiling data to compare this branch now?

@BenElferink BenElferink added the enhancement New feature or request label Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants