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

Include field name in decoding errors #3866

Merged
merged 2 commits into from
Jun 11, 2020
Merged

Conversation

jalvz
Copy link
Contributor

@jalvz jalvz commented Jun 10, 2020

Motivation/summary

Troubleshooting decoding errors is currently very hard because the error doesn't include the field name, and there are many call sites (and calls) that can return an error.

This adds the field name to the decoding error message.

Checklist

I have considered changes for:

How to test these changes

make test

@apmmachine
Copy link
Contributor

apmmachine commented Jun 10, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #3866 updated]

  • Start Time: 2020-06-11T06:35:41.026+0000

  • Duration: 47 min 49 sec

Test stats 🧪

Test Results
Failed 0
Passed 3210
Skipped 146
Total 3356

Steps errors

Expand to view the steps failures

  • Name: Test Sync
    • Description: ./script/jenkins/sync.sh

    • Duration: 3 min 44 sec

    • Start Time: 2020-06-11T06:47:28.044+0000

    • log

@axw
Copy link
Member

axw commented Jun 11, 2020

Can you give an example of an error where this would have helped? I'm wondering why we're getting these "fetch" errors at all - shouldn't JSON Schema validation have prevented them?

@jalvz
Copy link
Contributor Author

jalvz commented Jun 11, 2020

@axw they are relevant basically for development. One example for instance when you get it is if you call the wrong decoder method (eg. use decoder.String instead of decoder.StringPtr, etc)

@axw
Copy link
Member

axw commented Jun 11, 2020

@axw they are relevant basically for development. One example for instance when you get it is if you call the wrong decoder method (eg. use decoder.String instead of decoder.StringPtr, etc)

OK. So in this case it would be like the schema said something is optional, but in the code we made it required by using the wrong method. Fair enough.

@codecov-commenter
Copy link

Codecov Report

Merging #3866 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3866   +/-   ##
=======================================
  Coverage   79.80%   79.80%           
=======================================
  Files         135      135           
  Lines        6234     6234           
=======================================
  Hits         4975     4975           
  Misses       1259     1259           

@jalvz jalvz merged commit 603bcac into elastic:master Jun 11, 2020
jalvz added a commit to jalvz/apm-server that referenced this pull request Jun 11, 2020
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.

5 participants