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.

Sign up for GitHub

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

Jump to bottom

Database/sql instrumentation #240

Merged
merged 28 commits into from
Aug 22, 2023
Merged

Database/sql instrumentation #240

merged 28 commits into from
Aug 22, 2023

Conversation

RonFed
Copy link
Contributor

@RonFed RonFed commented Jul 27, 2023

This PR adds instrumentation for database/sql package.
Added additional example under examples/httpPlusDb
As a first step, this instrumentation only includes the query string.
Additional fields from the opentelemetry spec will be added in future PRs.
http_Db_traces

@RonFed RonFed requested a review from a team July 27, 2023 18:35
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 27, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

pellared
pellared reviewed Jul 28, 2023
View reviewed changes
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

E2E tests are missing.

I made only a quick look.

pkg/instrumentors/bpf/database/sql/probe.go Outdated Show resolved Hide resolved
pkg/instrumentors/bpf/database/sql/probe.go Show resolved Hide resolved
edeNFed
edeNFed approved these changes Jul 30, 2023
View reviewed changes
Copy link
Contributor

@edeNFed edeNFed left a comment

Choose a reason for hiding this comment

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

@RonFed looks great!

@pellared pellared mentioned this pull request Aug 1, 2023
Closed
RonFed and others added 2 commits August 2, 2023 10:34
@RonFed @pellared
Update pkg/instrumentors/bpf/database/sql/probe.go
9b284b6 
The SDK MUST interpret an empty value of an environment variable the same way as when the variable is unset.

Co-authored-by: Robert Pająk <pellared@hotmail.com>
@RonFed
Merge branch 'main' into database/sql_instrumentation
606ad10
pellared
pellared reviewed Aug 2, 2023
View reviewed changes
examples/httpPlusdb/test.db Outdated Show resolved Hide resolved
test/e2e/databasesql/test.db Outdated Show resolved Hide resolved
@RonFed
Copy link
Contributor Author

RonFed commented Aug 5, 2023

@pellared I resolved your comments, please let me know if there is anything else to fix

MikeGoldsmith
MikeGoldsmith suggested changes Aug 7, 2023
View reviewed changes
Copy link
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

This looks really good - thanks @RonFed 👍🏻

I've left some suggestions around parameter offsets. Also, it looks like the example app is all boiler plate - is there an official docker image published that could be used instead?

pkg/instrumentors/bpf/database/sql/bpf/probe.bpf.c Show resolved Hide resolved
pkg/instrumentors/bpf/database/sql/bpf/probe.bpf.c Outdated Show resolved Hide resolved
pkg/instrumentors/bpf/database/sql/bpf/probe.bpf.c Outdated Show resolved Hide resolved
pkg/instrumentors/bpf/database/sql/bpf/probe.bpf.c Outdated Show resolved Hide resolved
@RonFed
Copy link
Contributor Author

RonFed commented Aug 10, 2023

@MikeGoldsmith Can you please have another look ?

@pellared pellared requested a review from MikeGoldsmith August 10, 2023 15:42
MikeGoldsmith
MikeGoldsmith approved these changes Aug 22, 2023
View reviewed changes
Copy link
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @RonFed

@MikeGoldsmith MikeGoldsmith merged commit e814306 into open-telemetry:main Aug 22, 2023
@MrAlias MrAlias mentioned this pull request Aug 22, 2023
Closed
@MrAlias MrAlias added this to the v0.3.0-alpha milestone Aug 22, 2023
@RonFed RonFed mentioned this pull request Oct 11, 2023
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pellared pellared pellared left review comments

@edeNFed edeNFed edeNFed approved these changes

@MikeGoldsmith MikeGoldsmith MikeGoldsmith approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.3.0-alpha
Development

Successfully merging this pull request may close these issues.
5 participants
@RonFed @MikeGoldsmith @pellared @edeNFed @MrAlias