-
Notifications
You must be signed in to change notification settings - Fork 183
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
feat: Support including host name in faraday span names #1385
base: main
Are you sure you want to change the base?
feat: Support including host name in faraday span names #1385
Conversation
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.
Although I appreciate the challenges you're facing at the moment the instrumentation should have predictable low cardinality span names.
I don't have any alternatives for you at the moment in Ruby other than writing a custom span provessor but if there are any other @open-telemetry/ruby-contrib-approvers that may have opinions or suggestions on what we can offer I hope we can discuss them at the next SIG.
Hey @arielvalentin! I appreciate the quick response 😄 Could you elaborate a bit more on how my proposed changes differ from the alternate span naming conventions provided by other instrumentations such as Sidekiq or ActionController? To me, providing the option to include the request hostname in the span name doesn't feel like it would introduce significantly more variance in span names than what the alternates provided in other instrumentations already do. |
We follow the spec in those cases HTTP server instrumentations use low cardinality templated routes like Similarly Messaging/Job consumers and producers use the topic name they are use. Earlier versions of the instrumentations were implemented incorrectly and used class names. We kept them around for backward compatibility. The HTTP client libraries that we instrument here don't offer options like this and they would be closer to what is expected by the specification. Unless my knowledge is out of date, the same low cardinality rules should apply. A templated route or the http method are the recommended span names. Using the host name may result in high cardinality values. In these cases I would recommend using the collector to transform span names but that may not be an option for everyone. Another option would be to change the faraday instrumentation that would ensure it was the first middleware in the stack and then you can add a custom middleware to rename the span. There are other options but they open up potential challenges for "foot-guns" so I'd rather not even mention them 😔 I hope that helps clarify my concerns. I'm interested in what others think about this. |
Understood! Thanks for explaining your thought process some more 😄 I'll keep an eye on this thread to see what others have to say! |
Faraday span names appear as just "HTTP <METHOD>" which doesn't provide a ton of context when visualizing the spans.
For some specific context about our use case:
We use Grafana span metrics to create alerts when any of our critical 3rd-party APIs we use have a high latency. But since those metrics don't include the
net.peer.name
attribute that were on the original span, we only really have the span name to filter down the metrics to what we're looking for.Including the host name in the span name makes this process significantly easier for us and makes it much easier to identify what API call was performed at a quick glance rather than having to click into the span's attributes each time 😄