Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Add ContratName on Request.Name and Operation.Name #148

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LePastis
Copy link

When sending a telemetry Request, informations are send to ApplicationInsights, the requestName only contains the method name, and not ContractName. ex. I will have the following name: "Get", instead of "IClientApplicationService.Get".
So it's complicated to identify appropriate request.

The current pull request resolve the issue.

@tomasr
Copy link
Contributor

tomasr commented Apr 26, 2018

It's a good suggestion, but looking back at the code, I don't think this is quite the right way to fix it.

I'm also curious as to why you're seeing just the operation name being written. The OperationNameTelemetryInitializer should be overwriting RequestTelemetry.Name regardless of what the value set in RequestTrackingTelemetryModule, and that one does set it to Contract.Operation already (which is what you want).

Based on this, I think the right fix would be not to set a value on RequestTelemetry.Name in the first place, and just let the initializers handle it.

@LePastis
Copy link
Author

Yes, you are right the OperationNameTelemetryInitializer overwrite the RequestTelemetry.Name but only when the telemetry.Context.Operation.Name is null, but the RequestTrackingTelemetryModule initialize the Context.Operation.Name before calling initializer.

I agree with you, it's better if the initializer handle it but i can't identify the impact of deleting the controle before overwriting RequestTelemetry.Name.

@tomasr
Copy link
Contributor

tomasr commented Apr 26, 2018

Hmm.... good point. If the RequestTelemetry object was picked up from an existing HttpContext associated with the thread (which will happen if ASP.NET Hosting Compability is enabled), then we could be failing to set RequestTelemetry.Name properly if relying only on the telemetry initializers. But I think that could be fixed by moving this block outside of the outer if block.

Does that fit what you're seeing right now?

Honestly, changing the RequestTrackingTelemetryModule like you suggest shouldn't be a big deal, I just wanted to avoid adding one more memory allocation per request.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants