-
Notifications
You must be signed in to change notification settings - Fork 319
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 ignore on request path #2146
Fix ignore on request path #2146
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
There was a problem hiding this 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.
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.when; | ||
|
||
class ServletTransactionCreationHelperTest extends AbstractInstrumentationTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
.../src/test/java/co/elastic/apm/agent/servlet/helper/ServletTransactionCreationHelperTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: eyalkoren <41850454+eyalkoren@users.noreply.github.com>
After blaming the CI, I found what was causing the timeouts. |
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