GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — observability: fix err access scope to correctly retrieve it on defer by otternq · Pull Request #43 · opencensus-integrations/ocsql

Conversation

otternq commented May 17, 2020

Copy link

Copy Markdown

Contributor

Ensure the final err value is reported to recordCallStats' annonymous function instead of nil.

Stats were always reporting GoSQLStatus as OK even when queries failed. This change will allow the status to be ERROR and for GoSQLError to be populated.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

odeke-em requested changes

May 17, 2020

View reviewed changes

odeke-em left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thank you for catching this bug @otternq and for submitting this PR.
I have posted up some suggestions, please take a look. Could you also update
the commit subject and message to

observability: fix err access scope to correctly retrieve it on defer

Fixes a bug in which we mistakenly used the earliest value of err
which was always nil, when measuring latency. The fix here is
to invoke the latency measuring closure inside a defer to correctly
capture the latest value of the named err value.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

driver.go Outdated

Comment on lines +136 to +141

		var recordCallStatsFunc = recordCallStats(ctx, "go.sql.ping", c.options.InstanceName)
		defer func() {
		recordCallStatsFunc(err)
		}()

odeke-em May 17, 2020

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

    onDeferWithErr := recordCallStats(ctx, "go.sql.ping", c.options.InstanceName)
    defer func() {
          // Invoking this function in a defer so that we can capture
          // the value of err as set on function exit.
          onDeferWithErr(err)
    }()

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          observability: fix err access scope to correctly retrieve it on defer

9f4c7ea

Fixes a bug in which we mistakenly used the earliest value of err
which was always nil, when measuring latency. The fix here is
to invoke the latency measuring closure inside a defer to correctly
capture the latest value of the named err value.

otternq force-pushed the record-call-stats branch from ec0b126 to 9f4c7ea Compare

May 17, 2020 23:57

otternq commented May 18, 2020

Copy link

Copy Markdown

Contributor Author

Thanks for the review @odeke-em, I've changed the commit message and applied your suggestions.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

odeke-em changed the title ~~Change err scoping for recordCallStats call~~ observability: fix err access scope to correctly retrieve it on defer

May 18, 2020

odeke-em approved these changes

May 18, 2020

View reviewed changes

odeke-em left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Awesome, thank you @otternq, LGTM!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

odeke-em merged commit b6f4ab8 into opencensus-integrations:master

May 18, 2020

odeke-em commented May 18, 2020

Copy link

Copy Markdown

Member

And thank you again @otternq, I have cut release https://github.com/opencensus-integrations/ocsql/releases/tag/v0.1.6

@a8m @yancl @lwc y'all might wanna upgrade to the latest release, thanks!

a8m reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

yancl commented May 18, 2020

Copy link

Copy Markdown

Contributor

Thanks for your info, @odeke-em :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Reviewers

odeke-em

odeke-em approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.