contrib/dimfeld/httptreemux/v5: add support for httptreemux #1546
Conversation
|
Hi friendly folks! 👋 Just wanted to check if there is anything needed from us for the time being? Looking forward to your initial review! 🙌 |
|
Hello! The team does periodic proposal reviews where we go through the github issues marked with |
|
I would really love to see this PR merged |
|
We just looked at the proposal and accepted it! We'll do a full review of this PR soon :) |
| if !math.IsNaN(cfg.analyticsRate) { | ||
| cfg.spanOpts = append(cfg.spanOpts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate)) | ||
| } | ||
| cfg.spanOpts = append(cfg.spanOpts, tracer.Measured()) |
There was a problem hiding this comment.
A few more spanOpts to add:
cfg.spanOpts = append(cfg.spanOpts, tracer.Tag(ext.SpanKind, ext.SpanKindServer))
cfg.spanOpts = append(cfg.spanOpts, tracer.Tag(ext.Component, "dimfeld/httptreemux.v5"))
https://github.com/DataDog/dd-trace-go/tree/main/contrib#usage
There was a problem hiding this comment.
Same thing for ContextRouter
There was a problem hiding this comment.
- add missing
spanOptstoRouter - add missing
spanOptstoContextRouter
There was a problem hiding this comment.
All of this code should be in contrib/dimfeld/httptreemux.v5/ per our naming conventions: https://github.com/DataDog/dd-trace-go/tree/main/contrib#usage
There was a problem hiding this comment.
- move
contrib/dimfeld/httptreemux/v5/tocontrib/dimfeld/httptreemux.v5/ - update references in log output
|
Thank you for the review and feedback @katiehockman! 🙌 I will work through the requested changes and submit an update as soon as possible. |
Adding support to trace the dimfeld/httptreemux/v5 router package. The changes are almost identical to those that were required to instrument julienschmidt/httprouter, since httptreemux is based on httprouter. Resolves DataDog#1520
Rename trace analytics enabled variable from DD_TRACE_HTTPROUTER_ANALYTICS_ENABLED to DD_TRACE_HTTPTREEMUX_ANALYTICS_ENABLED to correctly reflect what the underlying routing package is.
Add a default resource namer with the option to override it via the router configuration.
Update the defaultResourceNamer logic to exit early if the lookup fails to find a match. Change test case input to avoid confusion with the expected outcome.
Rename the TestDefaultResourceNamer to TestHttpTracer404 as this better reflects the intention.
This fixes an issue where the paramater substitution would fail if a parameter value was contained in the value of another. For example given the path '/foo/:x/:y/bar' and the request URL '/foo/123/2/bar', '2' is the value of parameter 'y' but also contained in the value of parameter 'x', i.e. '123. This could lead to a scenario where the resource name would be '/foo/1:y3/2/bar' after the substitution.
During review of the renamer change we discovered that the test could fail intermittently depending on the random iteration order of looping over items in the lookup params map. We now first check to replace parameters surrounded by a set of '/', i.e. '.../:param/...', and then move on to try and replace the parameter at the end of the path without a trailing '/', i.e. '../:param'. This fix worked even if we ran the test multiple times.
Add support for ContextMux provided with dimfeld/httptreemux by adding a new constructor function called NewWithContext. ContextMux adds the matched route and parameters to the request context.
Fixes a typo in the documentation of the ContextRouter struct.
devilleweppenaar
force-pushed
the
dv/1520/add-support-for-dimfeld-httptreemux
branch
from
d4912d2 to
76e877f
Compare
Co-authored-by: Katie Hockman <katie@hockman.dev>
Anylytics rate is deprecated and can be safely removed.
Add span.kind and component tags that are required to be set by all spans that are generated by integrations.
Move integration code from contrib/dimfeld/httptreemux/v5/ to contrib/dimfeld/httptreemux.v5/ to align with contribution guidelines.
Update comment and log statement references to use the correct naming scheme, i.e. contrib/dimfeld/httptreemux.v5.
|
Thanks again for the review and feedback @katiehockman. 🙌 I believe I have addressed all the points of feedback. Please take a look when you have a chance. 🙏 |
|
Looks like there are some lint errors, but if you could fix those @devillexio then we should be good to merge! |
Update to address linting error reported by goimports.
|
Merged, thanks again @devillexio !!! 🎉 |
devilleweppenaar
deleted the
dv/1520/add-support-for-dimfeld-httptreemux
branch
Adding support to trace the dimfeld/httptreemux/v5 router package. The changes are almost identical to those that were required to instrument julienschmidt/httprouter, since httptreemux is based on httprouter.
Resolves #1520.