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

feat: Support prepend SQL comment for PG instrumentation #690

Merged
merged 6 commits into from
May 10, 2024
Merged

feat: Support prepend SQL comment for PG instrumentation #690

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
676425c
feat: Support prepend SQL comment for PG instrumentation
keiko713 Oct 12, 2023
d9bf281
Merge branch 'main' into pg-prepend-comment
arielvalentin Feb 8, 2024
031fdca
Merge branch 'main' into pg-prepend-comment
arielvalentin Feb 8, 2024
ca71e53
Merge branch 'main' into pg-prepend-comment
kaylareopelle Mar 12, 2024
063ccad
Merge branch 'main' into pg-prepend-comment
kaylareopelle Apr 1, 2024
515df0d
Merge branch 'main' into pg-prepend-comment
kaylareopelle May 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion instrumentation/pg/lib/opentelemetry/instrumentation/pg/patches/connection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,9 @@ def span_attrs(kind, *args)

def extract_operation(sql)
# From: https://github.com/open-telemetry/opentelemetry-js-contrib/blob/9244a08a8d014afe26b82b91cf86e407c2599d73/plugins/node/opentelemetry-instrumentation-pg/src/utils.ts#L35
sql.to_s.split[0].to_s.upcase
# Ignores prepend comment
comment_regex = %r{\A\/\*.*?\*\/}m
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to move this into constant?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I concur with @simi

@keiko713 Please extract this regex into a constant to avoid allocating it for every query.

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL Ruby freezes Regexes like it freezes strings.

Running a file named foo.rb with the following ruby code:

# frozen_string_literal: true

def foo
  %r{\A\/\*.*?\*\/}m.object_id
end

def bar
  'hello'.object_id
end

puts foo
puts foo
puts foo

puts bar
puts bar
puts bar

has this result:

➜  ~ ruby foo.rb
60
60
60
80
80
80

So I'm going to merge this in without moving it to a constant.

sql.to_s.sub(comment_regex, '').split[0].to_s.upcase
Copy link
Collaborator

Choose a reason for hiding this comment

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

To confirm, we are only interested in removing the first set of long form comments (without newlines) like those generated in SQL Commenter format.

We are also not concerned about inline comments -- this is a comment

Is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's my understanding since the goal of this method is to grab the operation (ex. SELECT), so any comments after the operation won't apply. I'm not sure what would happen if there were two comments back-to-back before the operation, but that seems pretty unlikely.

end

def span_name(operation)
Expand Down
12 changes: 12 additions & 0 deletions instrumentation/pg/test/opentelemetry/instrumentation/pg/instrumentation_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,18 @@
end
end

it 'ignores prepend comment to extract operation' do
client.query('/* comment */ SELECT 1')

_(span.name).must_equal 'SELECT postgres'
_(span.attributes['db.system']).must_equal 'postgresql'
_(span.attributes['db.name']).must_equal 'postgres'
_(span.attributes['db.statement']).must_equal '/* comment */ SELECT 1'
_(span.attributes['db.operation']).must_equal 'SELECT'
_(span.attributes['net.peer.name']).must_equal host.to_s
_(span.attributes['net.peer.port']).must_equal port.to_i
end

it 'only caches 50 prepared statement names' do
51.times { |i| client.prepare("foo#{i}", "SELECT $1 AS foo#{i}") }
client.exec_prepared('foo0', [1])
Expand Down