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: avoid panic when sourcemap doc does not contain hit #4619

Merged
merged 3 commits into from
Jan 14, 2021

Conversation

simitt
Copy link
Contributor

@simitt simitt commented Jan 13, 2021

Motivation/summary

Apparently it can happen that the returned ES doc indicates a sourcemap was fetched but the actual hit does not contain one. This is unexpected and should not happen, therefore it would be interesting how to reproduce and figure out if there is another underlying issue.
Nevertheless, APM Server should not panic, so let's get this PR in to fix it.

Checklist

How to test these changes

Let's wait if we can get more input from the user reporting the issue on how to reproduce.

Related issues

closes #4601

@simitt simitt requested a review from a team January 13, 2021 12:40
@apmmachine
Copy link
Contributor

apmmachine commented Jan 13, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #4619 updated

    • Start Time: 2021-01-14T12:22:13.746+0000
  • Duration: 42 min 56 sec

  • Commit: e8049da

Test stats 🧪

Test Results
Failed 0
Passed 4607
Skipped 124
Total 4731

Steps errors 2

Expand to view the steps failures

Compress
  • Took 0 min 0 sec . View more details on here
  • Description: tar --exclude=coverage-files.tgz -czf coverage-files.tgz coverage
Compress
  • Took 0 min 0 sec . View more details on here
  • Description: tar --exclude=system-tests-linux-files.tgz -czf system-tests-linux-files.tgz system-tests

@codecov-io
Copy link

Codecov Report

Merging #4619 (e8049da) into master (374f287) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4619      +/-   ##
==========================================
+ Coverage   75.74%   75.76%   +0.02%     
==========================================
  Files         162      162              
  Lines        9815     9815              
==========================================
+ Hits         7434     7436       +2     
+ Misses       2381     2379       -2     
Impacted Files Coverage Δ
sourcemap/es_store.go 86.36% <100.00%> (+3.03%) ⬆️
...ack/apm-server/aggregation/txmetrics/aggregator.go 93.36% <0.00%> (ø)

@simitt simitt merged commit 5888fd3 into elastic:master Jan 14, 2021
simitt added a commit to simitt/apm-server that referenced this pull request Jan 14, 2021
@simitt simitt deleted the fix-sourcemap-panic branch August 20, 2021 07:34
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.

Crashed at sourcemap/es_store.go
4 participants