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

API to make using the request activity easier #11715

Closed
JamesNK opened this issue Jun 29, 2019 · 6 comments · Fixed by #31180
Closed

API to make using the request activity easier #11715

JamesNK opened this issue Jun 29, 2019 · 6 comments · Fixed by #31180
Assignees
Labels
affected-medium This issue impacts approximately half of our customers area-hosting Includes Hosting area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Jun 29, 2019

Hosting sets up an activity for each request (if logging or the diagnostic listener is enabled) called Microsoft.AspNetCore.Hosting.HttpRequestIn

Web frameworks may want to add additional data to the activity as tags for telemetry frameworks who use the activity. For example, gRPC uses a custom status trailer instead of HTTP status. Adding the status trailer to the activity would allow telemetry frameworks will recognize the request as gRPC and report the status.


Getting the request activity today is a little difficult. App frameworks would need to get the current activity from the static Activity.Current, then loop through the hierarchy looking for an activity called Microsoft.AspNetCore.Hosting.HttpRequestIn. Once the tag has been set the framework would then need to notify the listener that the activity has been changed so telemetry frameworks can use the change immediately. DiagnosticListener isn't the easiest API to use correctly.

It would be easier if there was an API for this, that was scoped per request. Something like:

public interface IRequestActivityAccessor
{
    Activity Activity { get; }
    NotifyActivityChanged();
}
_requestActivity.Activity.AddTag("grpc.status_code", Success);
_requestActivity.NotifyActivityChanged();

An API like this would make it much easier for ASP.NET Core frameworks to reuse and enrich the existing activity, and provide a common pattern for telemetry frameworks to consume.

@davidfowl @rynowak @lmolkova

@davidfowl
Copy link
Member

Do we need NotifyActivityChanged? Otherwise this could just be a property on the HttpContext. Then as we add various events during the request pipeline the that send the HttpContext consumer can look at the activity to see what changed.

This becomes a simple HttpContext.Activity (via some feature interface, IHttpActivityFeature).

public interface IHttpActivityFeature
{
    Activity Activity { get; set; }
}

@JamesNK
Copy link
Member Author

JamesNK commented Jun 30, 2019

Do we need NotifyActivityChanged?

I don't think we need it. In gRPC's case we want to add metadata after the request is starting and before it is ending.

The endpoint routing event you added is a good place for metadata at request start, and we don't need an event at the end because hosting raises one when it stops the activity. If app frameworks add other metadata to the activity then I guess they can raise there own specific events.

@davidfowl davidfowl self-assigned this Jun 30, 2019
@davidfowl davidfowl added this to the 3.0.0-preview8 milestone Jun 30, 2019
@davidfowl davidfowl added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jul 22, 2019
@analogrelay
Copy link
Contributor

This seems to have gotten lost and time is out. Moving to the backlog, we can consider for 3.1 or 5.0.

@analogrelay analogrelay modified the milestones: 3.0.0-preview8, Backlog Jul 23, 2019
@mkArtakMSFT mkArtakMSFT removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Nov 4, 2019
@Tratcher Tratcher added affected-medium This issue impacts approximately half of our customers enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool labels Nov 9, 2020 — with ASP.NET Core Issue Ranking
@davidfowl
Copy link
Member

A feature would be better for this. @shirhatti Thoughts?

@shirhatti
Copy link
Contributor

I like it. Let's add a feature

@shirhatti shirhatti removed this from the Backlog milestone Mar 23, 2021
@BrennanConroy BrennanConroy assigned shirhatti and unassigned davidfowl Mar 26, 2021
@ghost
Copy link

ghost commented Mar 31, 2021

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@dotnet dotnet locked as resolved and limited conversation to collaborators Jul 4, 2021
@amcasey amcasey added the area-hosting Includes Hosting label Jun 1, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-medium This issue impacts approximately half of our customers area-hosting Includes Hosting area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants