Skip to content

Fix ignore on request path #2146

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Sep 23, 2021
Merged

Fix ignore on request path #2146

merged 10 commits into from
Sep 23, 2021

Conversation

SylvainJuge
Copy link
Member

@SylvainJuge SylvainJuge commented Sep 22, 2021 •
edited
Loading

What does this PR do?

Currently we ignore request using Servlet path, which does not include the Servlet context path.
As a result, trying to ignore URLs with /app-context/* with a web-application deployed to the /app-context context path is not ignored.

Checklist

  • This is a bugfix
@ghost
Copy link

ghost commented Sep 22, 2021 •
edited by ghost
Loading

💚 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 preview

Expand to view the summary

Build stats

  • Start Time: 2021-09-23T15:52:20.948+0000

  • Duration: 57 min 54 sec

  • Commit: b98ebcc

Test stats 🧪

Test Results
Failed 0
Passed 2416
Skipped 19
Total 2435

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

  • run compatibility tests : Run the JDK Compatibility test.

  • run integration tests : Run the APM-ITs.
@SylvainJuge SylvainJuge marked this pull request as ready for review September 22, 2021 14:48
eyalkoren
eyalkoren reviewed Sep 23, 2021
View reviewed changes
Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better!
Some wording proposals and one test enhancement proposal, otherwise looks good!

For some reason, integration tests in the latest build took considerably longer than expected and were aborted eventually.
I started another build, once it passes properly, you can see this PR as approved.
.../src/test/java/co/elastic/apm/agent/servlet/helper/ServletTransactionCreationHelperTest.java
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

class ServletTransactionCreationHelperTest extends AbstractInstrumentationTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️
SylvainJuge and others added 2 commits September 23, 2021 10:28
@SylvainJuge @eyalkoren
Apply suggestions from code review
499c2a3 
Co-authored-by: eyalkoren <41850454+eyalkoren@users.noreply.github.com>
@SylvainJuge
post-review changes
3f99609
@SylvainJuge SylvainJuge self-assigned this Sep 23, 2021
@SylvainJuge
Copy link
Member Author

After blaming the CI, I found what was causing the timeouts.
There was a one character typo in the URL generated to poll for application status, which in turn generated invalid URLs, which could not be polled 🤦 .
@SylvainJuge SylvainJuge merged commit 7b52d0e into elastic:master Sep 23, 2021
@SylvainJuge SylvainJuge deleted the fix-ignore-request-path branch September 23, 2021 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-java

2 participants

@SylvainJuge @eyalkoren