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 an API for writing inline annotations that apply to a subset of queries #81

Merged
merged 3 commits into from
Mar 15, 2019

Conversation

mattyoho
Copy link
Contributor

Marginalia's request-level or job-level annotations are great, but in some cases it's valuable to add annotations to a specific subset of queries issued during a request or other context.

Since Marginalia is already hooked into annotating the SQL string passed into the database adapter level, it provides a good chokepoint for that functionality as well.

This patch implements a new API to add that ability. The following code:

Marginalia.with_annotation("foo") do
  Account.where(queenbee_id: 1234567890).first
end

will issue this query:

Account Load (0.3ms)  SELECT `accounts`.* FROM `accounts`
WHERE `accounts`.`queenbee_id` = 1234567890
LIMIT 1
/*application:BCX,controller:project_imports,action:show*/ /*foo*/

Any queries issues inside the with_annotation block will include the inline comment.

Nesting with_annotation blocks will concatenate the comment strings.

Marginalia.with_annotation("foo") do
  Marginalia.with_annotation("-bar") do
    Account.where(queenbee_id: 1234567890).first
  end
end

yields:

Account Load (0.3ms)  SELECT `accounts`.* FROM `accounts`
WHERE `accounts`.`queenbee_id` = 1234567890
LIMIT 1
/*application:BCX,controller:project_imports,action:show*/ /*foo-bar*/

/cc @arthurnn

@mattyoho
Copy link
Contributor Author

Note: the current build failures appear to be in the test setup itself, and are occurring on master as well.

@eileencodes
Copy link
Contributor

Hey @mattyoho can you rebase with master? I fixed the Travis tests to run and to test against 5.2 additionally.

@mattyoho
Copy link
Contributor Author

@eileencodes Awesome, thank you! Rebased. 👍

Copy link
Collaborator

@arthurnn arthurnn 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, i am ready to merge it when you are

@@ -30,6 +30,11 @@ def self.construct_comment
ret
end

def self.construct_inline_comment
return nil if inline_annotations.none?
escape_sql_comment(inline_annotations.join)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 for escaping it

@mattyoho
Copy link
Contributor Author

i am ready to merge it when you are

Awesome. I think this diff is ready. 👍

@arthurnn arthurnn merged commit 9e240b8 into basecamp:master Mar 15, 2019
@arthurnn
Copy link
Collaborator

new version released also https://rubygems.org/gems/marginalia/versions/1.7.0

@mattyoho
Copy link
Contributor Author

Great, thanks!

mattyoho added a commit to mattyoho/marginalia that referenced this pull request Mar 18, 2019
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.

3 participants