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

fix: get correct table name if table name is quoted #1234

Merged
merged 7 commits into from
Nov 24, 2024

Conversation

HielkeJ
Copy link
Contributor

@HielkeJ HielkeJ commented Nov 6, 2024

TABLE_NAME is extracted if the table name is quoted. Should fix #1173

Copy link
Collaborator

@arielvalentin arielvalentin left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.

@kaylareopelle @hannahramadan would you all mind comparing that to the new relic implementation to see if we are missing anything else?

@hannahramadan
Copy link
Contributor

hannahramadan commented Nov 8, 2024

Thanks @HielkeJ!

I'm wondering if we should remove the single quotes from the regex. @mostfunkyduck pointed out that single quotes are for string literals, not identifiers. What do you think @arielvalentin @scbjans

New Relic does not have this implemented and I didn't see others who did, so unfortunately not a lot to compare to.

@scbjans
Copy link
Contributor

scbjans commented Nov 9, 2024

Thanks for pointing that out, I somehow read past that.
After reading those docs, I agree that the single quotes should be removed from the regex, since those aren't for identifiers.

@HielkeJ
Copy link
Contributor Author

HielkeJ commented Nov 10, 2024

I did a small update in the PR @hannahramadan. If I try in Postgres to to a select SELECT * FROM 'table', it returns a syntax error. I didn't test if it fails in opentelemetry-ruby-contrib as well with a syntax error, but at least in shouldn't run the query.

@arielvalentin
Copy link
Collaborator

Given this new information I'm going to rescind my approval and scrutinize this a little more.

I'll leave the final approval to some other folks.

Copy link
Contributor

@hannahramadan hannahramadan 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 good to me with the recent updates.

@arielvalentin arielvalentin merged commit 192b262 into open-telemetry:main Nov 24, 2024
57 checks passed
@github-actions github-actions bot mentioned this pull request Nov 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PG instrumentation does not parse quoted table names in queries into the 'db.collection.name' attribute
5 participants