contrib/go-chi: customize span resource name for go-chi #1950
Conversation
opsengine
force-pushed
the
chi-custom-resource-name
branch
2 times, most recently
from
b3d2bc0
to
f9e0b42
Compare
opsengine
force-pushed
the
chi-custom-resource-name
branch
3 times, most recently
from
3d13029
to
76e54ae
Compare
opsengine
force-pushed
the
chi-custom-resource-name
branch
2 times, most recently
from
1f9bb73
to
d3df1a6
Compare
contrib/go-chi/chi/option.go
Outdated
| // WithResourceName specifies a function to use for determining the resource | ||
| // name of the span. | ||
| func WithResourceName(fn func(r *http.Request) string) Option { |
There was a problem hiding this comment.
Convention based on other integrations is to call this WithResourceNamer.
Example: https://pkg.go.dev/gopkg.in/DataDog/dd-trace-go.v1@v1.50.1/contrib/urfave/negroni#WithResourceNamer
(here and throughout, so line 28 would be resourceNamer instead
contrib/go-chi/chi/chi_test.go
Outdated
There was a problem hiding this comment.
I don't see a test for the "unknown" resource name case. I realize this isn't new code that you added, but if it's feasible for you to add a test to ensure that it works the same as it did before in that case, it would be great.
There was a problem hiding this comment.
I also unfortunately don't see a test for checking the ext.HTTPRoute, or the default resource name either 😢
Would it be possible for you to write those up? If not, let us know and I can help you with that.
opsengine
force-pushed
the
chi-custom-resource-name
branch
from
a6a3191
to
d3df1a6
Compare
opsengine
force-pushed
the
chi-custom-resource-name
branch
from
d3df1a6
to
7b38bc3
Compare
|
@katiehockman thanks for the feedback. I added the missing tests and fixed code conventions. |
|
Hi @opsengine,Thank you for your valuable contribution. It's important to note that v5 already has a WithModifyResourceName function, so we'd need to ensure compatibility with this existing feature. @katiehockman, I would appreciate your thoughts on this matter too. |
I looked at
Not for the moment. |
|
Thanks for clarifying the limitations of Given the widespread usage of chi.v5, I'm considering extending this enhancement to it. If you're open to it, I'd like to create a new PR to make this possible. |
|
Yes, please go ahead :) |
|
@katiehockman good to merge? |
There was a problem hiding this comment.
LGTM! Thank you @opsengine for your contribution and it sounds like a great idea @taowata to extend similar functionality to v5 as well as you mentioned here
Co-authored-by: Angelo Marletta <angelo.marletta@coinbase.com> Co-authored-by: Zarir Hamza <zarir.hamza@datadoghq.com>
What does this PR do?
Let the caller define how the resource name should be constructed, given the
http.Requestobject. IfWithResourceNameis not used, the old behavior is preserved.Motivation
Improve span resource names.
Describe how to test/QA your changes
Unit tests
Reviewer's Checklist