Skip to content
This repository was archived by the owner on Mar 19, 2019. It is now read-only.

Implementing IRequestIdentifierFeature #94

Merged
merged 1 commit into from
Mar 4, 2015
Merged

Implementing IRequestIdentifierFeature #94

merged 1 commit into from
Mar 4, 2015

Conversation

Praburaj
Copy link
Contributor

@Praburaj Praburaj commented Mar 3, 2015

Using code from HttpListener codebase to generate trace ids just to be consistent with other code.

Depends on aspnet/HttpAbstractions#210.

@Tratcher @GrabYourPitchforks

get
{
var guid = new Guid();
*(1 + (ulong*)&guid) = Request.RequestId;
Copy link
Contributor

Choose a reason for hiding this comment

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

What am I missing here? Why unsafe pointer arithmetic?

Copy link
Member

Choose a reason for hiding this comment

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

HttpListener has this exact code and somebody was asking about it the other day. Nobody knows.

Copy link
Contributor

Choose a reason for hiding this comment

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

I honestly don't even understand it. Why the 1 +? But ultimately why not just grab the RequestId directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unsafe is because it has memory pointer operations. I heard from @GrabYourPitchforks that the RequestId is a memory address and traditionally IIS and HttpListener has been overlaying this memory address on a base Guid instead of generating a random Guid for every request (as Guid generation is expensive and it can be in such critical code paths).

IIS code : http://wincode//View.aspx?File=%2F%2Fdepot%2Ffbl_srv2_iis%2Finetsrv%2Fiis%2Fiisrearc%2Fiis70%2Fcore%2Fw3context.cxx%236&SDPort=inetsrvdepot.sys-ntgroup.ntdev.microsoft.com%3A2009&Catalog=fbl_srv2_iis%28dev%29&Query=gettraceactivityid
HttpListener code: http://referencesource.microsoft.com/#System/net/System/Net/HttpListenerRequest.cs,333

Copy link
Contributor

Choose a reason for hiding this comment

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

So this is to "share" the memory with IIS for the same GUID? Copying a few bytes is really that expensive?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think that's fine. I just don't understand why it's doing crazy pointer math instead of just copying the value. The value doesn't change during the request, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically my question is, why isn't the code this:

return (Guid)Request.RequestId;

(Or whatever would compile - I don't know the types.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

HTTP.SYS doesn't surface the tracing id as a first-class value. You can calculate it from the request id by following the same steps as in the IIS code you linked to above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Eilon @GrabYourPitchforks Updated the PR by using the same base guid as HTTP.SYS and logic similar to IIS in generating the tracing id.

@Praburaj
Copy link
Contributor Author

Praburaj commented Mar 4, 2015

@Eilon @GrabYourPitchforks is this change good to go! Could you sign off if its good?

@Eilon
Copy link
Contributor

Eilon commented Mar 4, 2015

Looks :shipit: to me.

Using code from HttpListener codebase to generate trace ids just to be consistent with other code.
@Praburaj Praburaj merged commit ca3259f into aspnet:dev Mar 4, 2015
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.

5 participants