-
Notifications
You must be signed in to change notification settings - Fork 193
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
Add ignore_urls to central config #872
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
|
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.
Looking good!
A couple of additional bits are needed:
- In
newTracer
, another call tot.setLocalInstrumentationConfig
is needed to set the initial local config. This is required so if remote config is set, and then later unset, the tracer will revert to the local config. - In
config_test.go
, add a subtest to TestTracerCentralConfigUpdate for "transaction_ignore_urls"
module/apmhttp/ignorer.go
Outdated
@@ -53,6 +56,15 @@ func DefaultServerRequestIgnorer() RequestIgnorerFunc { | |||
return defaultServerRequestIgnorer | |||
} | |||
|
|||
// DynamicServerRequestIgnorer returns the RequestIgnorer to use in | |||
// handlers. The list of wildcard patterns comes from central config | |||
func DynamicServerRequestIgnorer(t *apm.Tracer) RequestIgnorerFunc { |
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.
func DynamicServerRequestIgnorer(t *apm.Tracer) RequestIgnorerFunc { | |
func NewDynamicServerRequestIgnorer(t *apm.Tracer) RequestIgnorerFunc { |
Let's align the naming with the other functions below, since DefaultServerRequestIgnorer will be going away eventually.
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.
In fact, if you implement the suggestion above about changing Tracer.IgnoredTransactionURLMatchers to IgnoredTransactionURL, then we can just pass tracer.IgnoredTransactionURL
directly in as it will satisfy the function signature -- no need for (New)DynamicServerRequestIgnorer then.
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.
I realise now that RequestIgnorerFunc takes an *http.Request, whereas IgnoredTransactionURL takes a URL, so my second comment doesn't work. But let's please align the naming, and add New
to the front.
module/apmhttp/ignorer.go
Outdated
@@ -53,6 +56,15 @@ func DefaultServerRequestIgnorer() RequestIgnorerFunc { | |||
return defaultServerRequestIgnorer | |||
} | |||
|
|||
// DynamicServerRequestIgnorer returns the RequestIgnorer to use in | |||
// handlers. The list of wildcard patterns comes from central config | |||
func DynamicServerRequestIgnorer(t *apm.Tracer) RequestIgnorerFunc { |
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.
I realise now that RequestIgnorerFunc takes an *http.Request, whereas IgnoredTransactionURL takes a URL, so my second comment doesn't work. But let's please align the naming, and add New
to the front.
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.
LGTM once the extraneous test is removed. Thank you!
Co-authored-by: Andrew Wilkins <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #872 +/- ##
=======================================
Coverage 83.70% 83.70%
=======================================
Files 142 142
Lines 7934 7959 +25
=======================================
+ Hits 6641 6662 +21
- Misses 885 889 +4
Partials 408 408
Continue to review full report at Codecov.
|
Closes #814