Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jetty Plugin (sync) #771

Merged
merged 11 commits into from
Jul 31, 2015
Merged

Jetty Plugin (sync) #771

merged 11 commits into from
Jul 31, 2015

Conversation

kizizu
Copy link
Contributor

@kizizu kizizu commented Jul 24, 2015

This is a Jetty plugin that supports synchronous requests. #446
I've added my notes on the plugin in the wiki if anyone wants to take a look.

@kizizu kizizu changed the title #446 Jetty Plugin (sync) Jul 24, 2015
@@ -140,6 +140,8 @@ public static ProfilerConfig load(String pinpointConfigFileName) throws IOExcept
private boolean memcached = true;
private boolean memcachedKeyTrace = false;

private Filter<String> jettyExcludeUrlFilter = new SkipFilter<String>();
Copy link
Contributor

Choose a reason for hiding this comment

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

This field is not used anywhere. You'd better remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm not mistaken I needed this field as a default for filtering within the ServerHandleInterceptor:

        final String requestURI = request.getRequestURI();
        if (excludeUrlFilter.filter(requestURI)) {
            if (isTrace) {
                logger.trace("filter requestURI:{}", requestURI);
            }
            return null;
        }

Copy link
Contributor

Choose a reason for hiding this comment

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

excludedUrlFilter comes from JettyConfiguration not ProfilerConfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JettyConfiguration uses ProfilerConfig though:

    public JettyConfiguration(ProfilerConfig config) {
        final String jettyExcludeURL = config.readString("profiler.jetty.excludeurl", "");

        if (!jettyExcludeURL.isEmpty()) {
            this.jettyExcludeUrlFilter = new ExcludeUrlFilter(jettyExcludeURL);
        }
        else{
            this.jettyExcludeUrlFilter = new  SkipFilter<String>();
        }
    }

is this still not using the field I've added above?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's not using ProfilerConfig.jettyExcludeUrlFilter. Among members of ProfilerConfig, JettyConfiguration only uses readString(), which is not accessing jettyExcludedUrlFilter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Got jettyExcludeUrlFilter and jettyExcludeURL confused. Thank you!

@kizizu
Copy link
Contributor Author

kizizu commented Jul 28, 2015

@lioolli
Thank you for your feedback! I've tried covering everything you pointed out.

@dawidmalina
Copy link
Contributor

👍

lioolli added a commit that referenced this pull request Jul 31, 2015
@lioolli lioolli merged commit 840be80 into pinpoint-apm:master Jul 31, 2015
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants