Skip to content
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

Change HttpClient Span.Name From "Method Host" To "Method URI" #463

Closed
wants to merge 1 commit into from

Conversation

baiyunchen
Copy link

Currently, the .net core version http client records span.name = host, I think the span.name record is more reasonable as a URI. Because we are more concerned about which APIs are called and not which hosts when we analyze the call link.

Span.Name = "Method Host":
image

Span.Name = "Method URI":
image

@baiyunchen
Copy link
Author

I can not open jenkins build detail and can not see why the cannot be build,please help check!
This pr can build normal in my computer~

@gregkalapos gregkalapos self-requested a review August 30, 2019 14:45
@gregkalapos
Copy link
Contributor

@baiyunchen Thanks for the PR, I'm on it, no worries. Jenkins won't build until someone from the elastic org reviews it from security reasons - so no action needed from you.

I'm thinking about this, I'll get back soon with a response to the PR.

@@ -124,7 +124,7 @@ private void ProcessStartEvent(TRequest request, Uri requestUrl)

var currentExecutionSegment = _agent.Tracer.CurrentSpan ?? (IExecutionSegment)transaction;
var span = currentExecutionSegment.StartSpan(
$"{RequestGetMethod(request)} {requestUrl.Host}",
$"{RequestGetMethod(request)} {requestUrl.AbsolutePath}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this in more detail, and here is what I can say:

To get to the issue:

Because we are more concerned about which APIs are called and not which hosts when we analyze the call link.

You can figure this out by clicking on the method span - we show the whole URL which we set later in the cade here.

Screen Shot 2019-08-30 at 16 58 43

I know this is 1 click more than your solution, but the thinking behind this is to avoid overloading the UI - overall you definitely have the information you need.

Also, we try to standardize the span names across agents. We have this document stating that:

span name should have the format <method> <host>.

We also have this spreadsheet and as you can see there, all agents follow this format, except the Node.js agent - I’m trying to figure out why.

Now, specifically this line would introduce yet another format, so in this case it’d be <method> <path> and currently no other agent generates span name based on this format. I still think whether we should change this, but in case we do, I’d suggest to at least do what the Node.js agent does and also include the host, so the span name should be something like <method> <host><path>. I’m talking to other agent devs to figure out what we want to do here - happy to hear opinions on this.

@gregkalapos gregkalapos self-requested a review August 30, 2019 15:09
@gregkalapos gregkalapos self-assigned this Aug 30, 2019
@baiyunchen
Copy link
Author

Thanks Response.
I think that in the micro-service, the all apis will be called throught api gateway,and the timeline all display name looks like the same name.
I think the display of the uri is necessary regardless of the language.

@baiyunchen
Copy link
Author

Is it possible to allow customization how to record span name ?

@gregkalapos
Copy link
Contributor

Is it possible to allow customization how to record span name ?

Unfortunately currently I think we don't have anything that would give you this functionality. You can get the current span, here is how to do that:
https://www.elastic.co/guide/en/apm/agent/dotnet/current/public-api.html#api-current-span

So with that you could do something like:

Agent.Tracer.CurrentSpan.Name = "NewName";

But in this situation this is probably not feasible. We are thinking about introducing filters - similar to what other agents offer. With that you'd have access to spans before they are send to the APM Server and you could do modifications on them. But this is not implemented and we don't really know if we'll do it in .NET yet.

@baiyunchen
Copy link
Author

Is it possible to allow customization how to record span name ?

Unfortunately currently I think we don't have anything that would give you this functionality. You can get the current span, here is how to do that:
https://www.elastic.co/guide/en/apm/agent/dotnet/current/public-api.html#api-current-span

So with that you could do something like:

Agent.Tracer.CurrentSpan.Name = "NewName";

But in this situation this is probably not feasible. We are thinking about introducing filters - similar to what other agents offer. With that you'd have access to spans before they are send to the APM Server and you could do modifications on them. But this is not implemented and we don't really know if we'll do it in .NET yet.

Thanks Response,
Got It.

I think that if it is difficult to adjust default Span.Name in the HttpClient, we can allow customize Span.Name format in HttpClient, but the default Span.Name remains unchanged to avoid the trouble of design changes.

@gregkalapos
Copy link
Contributor

Hi @baiyunchen,

We discussed this issue to see how this would work across all agents. To get to the point quickly: we decided to keep the current behavior.

Here is why:
Given that these URLs can include dynamic path parameters, like /user/:id grouping those spans will became impossible. Unlike for incoming requests, for outgoing HTTP requests we can’t parse those parameters and put a template placeholder there, so we’d have span names like user/1, user/2 etc. The problem here is that having grouping for such spans is super useful and loosing it would limit what we can do with the data in the future. E.g. we had some discussions on having maybe metrics for spans, but once we’d introduce having path in the span name that becomes very hard to implement. So it’d be nice to be able to group on span names and having the path in case of spans for outgoing HTTP requests makes that hard or impossible.

We discussed other options like keeping the span name as it is, and maybe use a display name, or somehow compile the path into it when we show it on the UI. This is just an idea, and no work is done on it at the moment, but this would be a path that’d be probably a better approach.

So, if you don’t mind we’d close this issue and not merge. I hope the proposed solutions (either manually changing the span name if possible, or just relaying on the fact that with 1 additional click you see the URL) still help you.

Thanks,
Greg

@baiyunchen
Copy link
Author

Thanks for the reply, I respect your decision.

@baiyunchen baiyunchen closed this Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants