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

systemtest: migrate one sourcemap test to Go #4488

Merged
merged 1 commit into from
Dec 4, 2020

Conversation

axw
Copy link
Member

@axw axw commented Dec 3, 2020

Motivation/summary

Just migrating one for now, to simplify the R&D into moving source mapping to ingest node.

There were two identical (except for the names) Python system tests; I removed both. I suppose the "changed_index" test was meant to do something different, but right now it's just dead weight.

Checklist

I have considered changes for:
- [ ] documentation
- [ ] logging (add log lines, choose appropriate log selector, etc.)
- [ ] metrics and monitoring (create issue for Kibana team to add metrics to visualizations, e.g. Kibana#44001)

How to test these changes

cd systemtest && go test -v

Related issues

#3606

Just migrating one for now, to simplify the R&D
into moving source mapping to ingest node.
@apmmachine
Copy link
Contributor

💚 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 #4488 opened

  • Start Time: 2020-12-03T08:49:24.488+0000

  • Duration: 41 min 33 sec

Test stats 🧪

Test Results
Failed 0
Passed 4621
Skipped 139
Total 4760

Steps errors 4

Expand to view the steps failures

Run Window tests

  • Took 9 min 53 sec . View more details on here

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

Test Sync

  • Took 3 min 8 sec . View more details on here
  • Description: ./.ci/scripts/sync.sh

@axw axw merged commit cf8fef7 into elastic:master Dec 4, 2020
@axw axw deleted the sourcemap-systemtest branch December 4, 2020 01:25
simitt pushed a commit to simitt/apm-server that referenced this pull request Dec 15, 2020
Just migrating one for now, to simplify the R&D
into moving source mapping to ingest node.
simitt added a commit that referenced this pull request Dec 15, 2020
Just migrating one for now, to simplify the R&D
into moving source mapping to ingest node.

Co-authored-by: Andrew Wilkins <axw@elastic.co>
stuartnelson3 added a commit to stuartnelson3/apm-server that referenced this pull request Mar 4, 2021
timeouts configured at the server level result in
the request context being canceled; translate this
to a "server timeout" message before
logging/sending to the client.

the actual message being sent and status code
could also be changed, if there's a better way to
communicate what's happening, or a more
appropriate status code, we can change it. i'm not
sure at which point in the request/response
lifecycle it's failing, so it could be a 408 or
503.

note: this feels like a very broad approach.
reading around online did not provide a concrete
way to check that the request had timed out. an
alternative method, such as setting a
context.WithTimeout on the request for instead of
the server's ReadTimeout might give us more
visibility into the cause. my concern is that some
un-related error might cause the request (and
context) to be canceled, and then we report this
as a timeout.

fixes elastic#4488
stuartnelson3 added a commit to stuartnelson3/apm-server that referenced this pull request Mar 4, 2021
timeouts configured at the server level result in
the request context being canceled; translate this
to a "server timeout" message before
logging/sending to the client.

the actual message being sent and status code
could also be changed, if there's a better way to
communicate what's happening, or a more
appropriate status code, we can change it. i'm not
sure at which point in the request/response
lifecycle it's failing, so it could be a 408 or
503.

note: this feels like a very broad approach.
reading around online did not provide a concrete
way to check that the request had timed out. an
alternative method, such as setting a
context.WithTimeout on the request instead of the
server's ReadTimeout might give us more visibility
into the cause. my concern with the approach in
this PR is that some un-related error might cause
the request (and context) to be canceled, and then
we report this as a timeout.

fixes elastic#4488
axw pushed a commit to axw/apm-server that referenced this pull request Mar 25, 2021
timeouts configured at the server level result in
the request context being canceled; translate this
to a "server timeout" message before
logging/sending to the client.

the actual message being sent and status code
could also be changed, if there's a better way to
communicate what's happening, or a more
appropriate status code, we can change it. i'm not
sure at which point in the request/response
lifecycle it's failing, so it could be a 408 or
503.

note: this feels like a very broad approach.
reading around online did not provide a concrete
way to check that the request had timed out. an
alternative method, such as setting a
context.WithTimeout on the request instead of the
server's ReadTimeout might give us more visibility
into the cause. my concern with the approach in
this PR is that some un-related error might cause
the request (and context) to be canceled, and then
we report this as a timeout.

fixes elastic#4488
mergify bot pushed a commit that referenced this pull request Apr 27, 2021
timeouts configured at the server level result in
the request context being canceled; translate this
to a "server timeout" message before
logging/sending to the client.

the actual message being sent and status code
could also be changed, if there's a better way to
communicate what's happening, or a more
appropriate status code, we can change it. i'm not
sure at which point in the request/response
lifecycle it's failing, so it could be a 408 or
503.

note: this feels like a very broad approach.
reading around online did not provide a concrete
way to check that the request had timed out. an
alternative method, such as setting a
context.WithTimeout on the request instead of the
server's ReadTimeout might give us more visibility
into the cause. my concern with the approach in
this PR is that some un-related error might cause
the request (and context) to be canceled, and then
we report this as a timeout.

fixes #4488

(cherry picked from commit 506311f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants