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

Don't emit metric err on read err #1383

Merged
merged 3 commits into from Nov 13, 2019

Conversation

@gdbelvin
Copy link
Collaborator

gdbelvin commented Nov 12, 2019

Emitting the wrong number of dimensions will cause an error.

@gdbelvin gdbelvin requested a review from google/keytransparency as a code owner Nov 12, 2019
@googlebot googlebot added the cla: yes label Nov 12, 2019
@gdbelvin gdbelvin requested a review from libinjG Nov 12, 2019
@gdbelvin gdbelvin added the bug label Nov 12, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 12, 2019

Codecov Report

Merging #1383 into master will decrease coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1383      +/-   ##
==========================================
- Coverage    65.9%   65.72%   -0.19%     
==========================================
  Files          52       52              
  Lines        3980     3979       -1     
==========================================
- Hits         2623     2615       -8     
- Misses        960      963       +3     
- Partials      397      401       +4
Impacted Files Coverage Δ
core/sequencer/server.go 69.79% <ø> (-1.11%) ⬇️
core/sequencer/trillian_client.go 58.57% <0%> (-2.86%) ⬇️
core/integration/client_tests.go 84.77% <0%> (-0.47%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f626048...428b99d. Read the comment docs.

Copy link
Contributor

mhutchinson left a comment

Is it possible to test this and show the fix via a failing -> passing test?

Emitting the wrong number of dimensions will cause an error.
@gdbelvin gdbelvin force-pushed the gdbelvin:fixmetric branch from 39aad5f to 1c05be9 Nov 13, 2019
@gdbelvin

This comment has been minimized.

Copy link
Collaborator Author

gdbelvin commented Nov 13, 2019

I've now removed the offending line because the error is already covered by returning err.

I'm not sure it's worth the effort to fake out the reader and return an error. Especially since the metric function itself doesn't expose an error type.

@gdbelvin gdbelvin changed the title Emit metric with the right dimensions Don't emit metric err on read err Nov 13, 2019
@gdbelvin gdbelvin merged commit b74adc4 into google:master Nov 13, 2019
4 of 5 checks passed
4 of 5 checks passed
codecov/project 65.72% (-0.19%) compared to f626048
Details
GolangCI No issues found!
Details
Travis CI - Pull Request Build Passed
Details
cla/google All necessary CLAs are signed
codecov/patch Coverage not affected when comparing f626048...428b99d
Details
@gdbelvin gdbelvin deleted the gdbelvin:fixmetric branch Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.