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.

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

Added configuration for disabling the actuator trace filter #8650

Closed
wants to merge 1 commit into from
Closed

Added configuration for disabling the actuator trace filter #8650

wants to merge 1 commit into from

Conversation

ColinHarrington
Copy link
Contributor

@ColinHarrington ColinHarrington commented Mar 16, 2017 •
edited by philwebb
Loading

Added configuration for disabling the actuator trace filter (WebRequestTraceFilter).
This pattern already exists in MetricFilterAutoConfiguration but was missed in the TraceWebFilterAutoConfiguration. See Issue #8322 for more detail. Note: we've observed a measured overhead in Grails applications due to the WebRequestTraceFilter being in place when including the actuator dependency. Related: grails/grails-core#640

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 16, 2017
@ColinHarrington ColinHarrington force-pushed the disable-actuator-trace-filter branch from b235147 to f5aa215 Compare March 16, 2017 22:29
@wilkinsona
Copy link
Member

we've observed a measured overhead in Grails applications due to the WebRequestTraceFilter being in place

I can't see those measurements in grails/grails-core#640. Can you share them with us please?

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Mar 17, 2017
@graemerocher
Copy link
Contributor

@wilkinsona The user has posted the measurements in that thread, nevertheless I don't think this is new information that WebRequestTraceFilter adds overhead no? Issue #8322 seems to indicate that it is known that this is the case, so I'm not sure why the measurements are necessary.

@wilkinsona
Copy link
Member

The user has posted the measurements in that thread, nevertheless I don't think this is new information that WebRequestTraceFilter adds overhead no?

If there's concrete data that it's specfically the WebRequestTraceFilter that's adding overhead then that would be new information. I can't see any such data in the Grails issue. What have I missed?

Issue #8322 seems to indicate that it is known that this is the case

Unfortunately, the numbers in #8322 don't really indicate anything that we could act upon which is why the issue was closed. They were looking at the actuator as a whole, rather than focussing on WebRequestTraceFilter, and one of the three scenarios showed a drop in GC time when the actuator was added. In short, the best that we could make of those numbers was that there's something in Actuator that appears to increase GC time in some scenarios and decrease it in others.

This PR is different (and better, IMO), as it's proposing something that's focussed specifically on the filter and aligns with the existing option for the metrics filter. That said, I'm still interested in seeing any performance data that's been collected that focuses specifically on the filter.

@graemerocher graemerocher mentioned this pull request Mar 20, 2017
Closed
@ColinHarrington
Added configuration for disabling the actuator trace filter (WebReque…
b645a3b 
…stTraceFilter) following the pattern in MetricFilterAutoConfiguration spring-projects#8322
@philwebb
Copy link
Member

I'd also be interested in seeing some performance data, but I think this is generally useful enhancement regardless. It aligns nicely with MetricFilterAutoConfiguration

@philwebb philwebb added priority: normal type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Mar 20, 2017
@philwebb philwebb added this to the 1.5.3 milestone Mar 20, 2017
@graemerocher
Copy link
Contributor

@philwebb @wilkinsona I have asked the reporter to provide more data. Thanks.

@ColinHarrington ColinHarrington force-pushed the disable-actuator-trace-filter branch from f5aa215 to b645a3b Compare March 20, 2017 15:42
@demon101
Copy link

demon101 commented Mar 20, 2017 •
edited
Loading

@wilkinsona
We didn't make performance testing, but on our production we get small CPU fall after replacing by DummyTraceRepository (without sync)

InMemoryTraceRepository has synchronized block for every insertion (every request)
If you need WebRequestTraceFilter, better to implement TraceRepository with lock-free collection

The pull request -- only disabling of endpoit, not a WebRequestTraceFilter

@ColinHarrington
Copy link
Contributor Author

FYI, I rebased onto a passing build from master

@philwebb philwebb mentioned this pull request Mar 20, 2017
Closed
@philwebb
Copy link
Member

Thanks @ColinHarrington

@philwebb
Copy link
Member

Thanks for the info @demon101. I've raised #8679 to see if we can improve that.

@snicoll snicoll self-assigned this Apr 11, 2017
snicoll pushed a commit that referenced this pull request Apr 11, 2017
@ColinHarrington @snicoll
Add the ability to disable the trace filter
d3e2e22 
See gh-8650
snicoll added a commit that referenced this pull request Apr 11, 2017
@snicoll
Merge pull request #8650 from ColinHarrington:disable-actuator-trace-…
7905874 
…filter

* pr/8650:
  Polish "Add the ability to disable the trace filter"
  Add the ability to disable the trace filter
@snicoll snicoll closed this in 2ef318c Apr 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@snicoll snicoll

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
1.5.3
Development

Successfully merging this pull request may close these issues.
7 participants
@ColinHarrington @wilkinsona @graemerocher @philwebb @demon101 @snicoll @spring-projects-issues