-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
WIP: add span attributes to tracing module #7269
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
base: master
Are you sure you want to change the base?
Conversation
c309a32 to
10404e6
Compare
|
|
||
| ````bash | ||
| $ go test ./... | ||
| $ go test ./modules/caddyhttp/tracing/ |
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.
Also I thought for newbies like me, if you want to attract them 😅 this is a helpful first pointer after the build.
|
Thanks for the contribution! I'll ask if @hairyhenderson, who I know is very busy, might have a chance to look this over, since I don't know much about the metrics/tracing stuff. |
|
Thank you! That would be much appreciated. But also I think the tracing part is the simple one. I believe based on the test that the span attributes are already recorded correctly. But the hard part is the replacements. I don't think I found a nice example of how response placeholders are generated and used. I pushed a test expecting a default span attr from OTEL, but you can see it fail there. Additionally, I couldn't really use the other response placeholders out of the box either. So I wonder if I'm doing something wrong either in setting up the test to be realistic, or in the middleware code to make them unavailable. |
|
Hi! Gently pinging this, is there anyone else that might be able to lend a hand with a review @mholt? Implementers of other systems that have used placeholders etc. 🙏 |
|
Well, the use of placeholders / replacer looks good IMO. I'm just not qualified to approve the rest of the code changes 😅 |
Fixes #7261
Hi @mholt, if I may ping you, thanks for labeling the associated issue! I decided to try to contribute a fix, as the scope is quite well defined.
First, I think my initial suggestion in the ticket of using a flat directive would probably
be improved by using a map instead, more consistent to what Caddyfile already does:
tracing { span my-span-name span_attributes { attr1 value1 attr2 {placeholder} } }I'm disclosing that I have used Claude to generate most of the code in this PR as this is my first time touching Go in general, although naturally I reviewed the output and tried to understand it. So if holding my hand through the MR is too much, feel free to discard it altogether in favor of a better solution!
If not, there are a couple of things I wanted to ask about. I think given the tests, is pretty already in a reasonable place. Building from that, on a practical level if you have quick pointers:
Many thanks! I'm very excited to try out Caddy for real and hopefully switch to it for good.