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 incorrect query and args handling #11

Merged
merged 1 commit into from
Nov 1, 2018

Conversation

luna-duclos
Copy link
Owner

Closes #10

@aaslamin
Copy link
Contributor

Still plumbing this in my project to test, but looking at the changes I don't think this is what you want to do?

We still want to create the arg tag in our spans if the OmitArgs is set to false. Logging is a separate thing all together. In order to this, we must conditionally call span.SetLabel("args", formatArgs(args)). Maybe I am missing something obvious? 😅

@aaslamin
Copy link
Contributor

Update: I was able to build my project against your branch after battling it out a bit more about modules.

I can confirm this is not the behaviour we want as the tag is removed even if WithOmitArgs() is not set. Instead we want to conditionally remove the tag like it was being done in some places before. Also the query tag is removed all together:

image

sql.go Outdated
span.SetLabel("query", query)
if !c.OmitArgs {
span.SetLabel("args", formatArgs(args))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So...this was previously good! We don't want to remove this check as far as I understand. Instead we want to do this in other places which are adding the args tag such as ExecContext and QueryContext.

Copy link
Owner Author

Choose a reason for hiding this comment

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

That's odd ... the call to logQuery a bit later is supposed to take care of that, I'll look into why that is broken

@luna-md luna-md force-pushed the ld/fix-incorrect-query-and-args-handling branch from 29ae18d to 0aee700 Compare November 1, 2018 13:39
@luna-duclos
Copy link
Owner Author

Issues fixed, fix corrected, merging this :)

@luna-duclos luna-duclos merged commit 7efacf5 into master Nov 1, 2018
@luna-duclos luna-duclos deleted the ld/fix-incorrect-query-and-args-handling branch November 1, 2018 13:40
@aaslamin
Copy link
Contributor

😍 Yay, thanks Luna! 🏆

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