Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Fix missing err short assignment in Go Metrics #491

Merged
merged 1 commit into from
Dec 1, 2018
Merged

Fix missing err short assignment in Go Metrics #491

merged 1 commit into from
Dec 1, 2018

Conversation

bluk
Copy link
Contributor

@bluk bluk commented Dec 1, 2018

It seems that err is not declared properly for some of the code snippets. It seems that it was removed in #484 .

Some of the other changes also seemed unnecessary for some functions (like naming the error return value (terr error) but not using terr). One change that may want to be investigated further is:

https://github.com/census-instrumentation/opencensus-website/pull/484/files#diff-4d73bb710d0b193312291ce07a0b156cL620

where the log.Fatal(err) replaces return err. The log.Fatal will cause a panic and I believe does not work correctly with the intended behavior in line 686 ( https://github.com/census-instrumentation/opencensus-website/pull/484/files#diff-4d73bb710d0b193312291ce07a0b156cL686 ) where any returned error is checked to be io.EOF and normally returns.

Copy link
Member

@odeke-em odeke-em 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 the fix @bluk! LGTM /cc @hvent90

@odeke-em odeke-em merged commit 241c306 into census-instrumentation:master Dec 1, 2018
@bluk bluk deleted the fix-missing-err branch December 1, 2018 21:00
@odeke-em
Copy link
Member

odeke-em commented Dec 2, 2018

Some of the other changes also seemed unnecessary for some functions (like naming the error return value (terr error) but not using terr).

@bluk (terr error) is used when capturing metrics at the end of the function, it was named because we want to capture the ending error regardless of the statement, in the cases were (err error) would be taken
screen shot 2018-12-01 at 10 26 11 pm

@bluk
Copy link
Contributor Author

bluk commented Dec 2, 2018

Ahh right. I was looking at the earlier snippets where terr isn't used yet, but in the final end-to-end program, it is used. Cool.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants