Skip to content

Switch to using Span concurrent weak map in Vert.x 3 #2322

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

Merged
merged 1 commit into from
Dec 8, 2021
Merged

Switch to using Span concurrent weak map in Vert.x 3 #2322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...pm-vertx/apm-vertx3-plugin/src/main/java/co/elastic/apm/agent/vertx/v3/web/WebHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@
*/
package co.elastic.apm.agent.vertx.v3.web;

import co.elastic.apm.agent.collections.WeakConcurrentProviderImpl;
import co.elastic.apm.agent.impl.ElasticApmTracer;
import co.elastic.apm.agent.impl.GlobalTracer;
import co.elastic.apm.agent.impl.transaction.Transaction;
import co.elastic.apm.agent.sdk.weakconcurrent.WeakConcurrent;
import co.elastic.apm.agent.sdk.weakconcurrent.WeakMap;
import co.elastic.apm.agent.vertx.AbstractVertxWebHelper;
import io.vertx.core.Handler;
Expand All @@ -34,7 +34,7 @@ public class WebHelper extends AbstractVertxWebHelper {

private static final WebHelper INSTANCE = new WebHelper(GlobalTracer.requireTracerImpl());

static final WeakMap<HttpServerRequest, Transaction> requestTransactionMap = WeakConcurrent.buildMap();
static final WeakMap<HttpServerRequest, Transaction> requestTransactionMap = WeakConcurrentProviderImpl.createWeakSpanMap();

public static WebHelper getInstance() {
return INSTANCE;
Expand Down
2 changes: 1 addition & 1 deletion ...java/co/elastic/apm/agent/vertx/v3/web/http1/HttpServerRequestImplEndInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public ElementMatcher<? super TypeDescription> getTypeMatcher() {

@Override
public ElementMatcher<? super MethodDescription> getMethodMatcher() {
return named("doEnd").and(takesNoArguments());
return named("doEnd").or(named("onEnd")).and(takesNoArguments());
Copy link
Member

@AlexanderWert AlexanderWert Dec 8, 2021

Choose a reason for hiding this comment

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

did we miss this initially? onEnd is available in 3.9 but not in 3.6.
How did you notice this?
Maybe we need a unit test for making sure context gets cleaned up properly, WDYT?

Copy link
Contributor Author

@eyalkoren eyalkoren Dec 8, 2021

Choose a reason for hiding this comment

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

This is exactly what this map migration does- it triggers our existing global recycling test assertions, thus uncovering such cases.
What we had before is a map that holds references to spans without counting these references, so the Vert.x API could change, causing spans to remain in the map and still get recycled. Now that we count these map references, if you don't properly remove them from the map, the recycling assertion causes the v3-latest tests to fail, which is how I found that.

}

@Override
Expand Down