-
Notifications
You must be signed in to change notification settings - Fork 867
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
Added Http4SpanDecorator to apache-camel #4650
Added Http4SpanDecorator to apache-camel #4650
Conversation
protected String getHttpUrl(Exchange exchange, Endpoint endpoint) { | ||
Object url = exchange.getIn().getHeader(Exchange.HTTP_URL); | ||
if (url instanceof String) { | ||
return ((String) url).replace("http4", "http"); |
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 https://camel.apache.org/components/2.x/http4-component.html they also mention urls starting with https4://
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.
Nice catch! Thanks!
fixed
class Http4SpanDecorator extends HttpSpanDecorator { | ||
@Nullable | ||
@Override | ||
protected String getHttpUrl(Exchange exchange, Endpoint endpoint) { |
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.
This is mostly a copy-paste from the super-class. Can you instead introduce a new protected method into HttpSpanDecorator
, something like getProtocol
. Super-class will return http
, this class can override it and return http4
.
Even easier way is, probably, to pass this protocol prefix as constructor parameter to HttpSpanDecorator
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.
Hey good point! thanks!
Fixed
} | ||
String url = super.getHttpUrl(exchange, endpoint); | ||
if (url != null) { | ||
return url.replace(getProtocol(), getProtocol().substring(0, getProtocol().length() - 1)); |
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.
Why is this still needed?
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.
This is where the replacement from ("http4"/"https4") to "http/https" happens.
It's only uniq to http4 protocol, so it has to be done here i think
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.
But why this replacement is needed? If Camel uses such protocols, shouldn't we preserve this information?
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.
Maybe we should, but the actual url of the request is not http4, but http.
"http4://url" is not a valid address. Apache translates the url to "http://url".
I thought that the "http.url" tag should hold the real URL..
Maybe we could add another tag? Or you're thinking that it should remain http.url="http4://some-url"?
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 am actually not sure that I understand what is going on here at all :) To me it seems that io.opentelemetry.javaagent.instrumentation.apachecamel.decorators.HttpSpanDecorator#getHttpUrl
method should return url that is present in either exchange or endpoint. If this is the url that was requested from Camel, then I would think that whatever protocol that url uses should remain in http.url
attribute value without modifications.
@kubawach can you please express your opinion as the original author of Camel integration?
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.
From what I understand http4
is the name of the camel component, not the protocol/scheme - these are separate concepts. The actual used HTTP scheme used is still either http
or https
.
WDYT about having both getProtocol()
(maybe renamed to getScheme()
to match OTEL naming) and getComponent()
methods, and using the second for searching in endpointUri and the first for constructing http.url
?
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.
@mateuszrzeszutek That is correct.
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.
@iNikem what are your latest thoughts on this? I can see the benefit of capturing the url as it was passed in by the user (in this case http4://
). But I also see the benefit of modeling the actual downstream request (in this case http://
).
Since it could some time to get feedback on your spec issue, let's just pick one so we can unblock @osherv, and I will open a tracking issue in this repo to make sure we sync up once there's clarification in the spec.
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.
If we are going to follow up after spec clarification, then it does not matter for me which of the two options we choose now. We can go ahead with what is already in this PR.
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.
Just found this notification so to reinforce @mateuszrzeszutek findings - the idea was that http is the actual url, while http4 only camel component.
Thanks @osherv! |
* Added Http4SpanDecorator to apache-camel * Fixed CR * After spotless
No description provided.