Add support for HTTP(S) OTLP endpoint #4197
Conversation
ghost
added
the
dotnet-policy-service
bot
added
the
community-contribution
| private static async Task<byte[]> ReadFullyAsync(HttpContext context) | ||
| { | ||
| using var ms = new MemoryStream(); | ||
| await context.Request.Body.CopyToAsync(ms).ConfigureAwait(false); | ||
| return ms.ToArray(); | ||
| } |
There was a problem hiding this comment.
This should be using pipelines to buffer without allocating a memory stream and a byte[]
There was a problem hiding this comment.
How do I achieve this?
Google.Protobuf.MessageParser<T> wants either ReadOnlySequence<byte> , ReadOnlySpan<byte> or a Stream. but the HttpContext.Request.Body-Stream can not be passed directly
There was a problem hiding this comment.
@davidfowl I got rid of the byte[] and stream allocation
|
The formatting changes make it really hard to review. I don't know if you can undo those but with a big change like this it really makes the diff bigger than it needs to be. |
I reverted the format changes. Sorry for that |
|
Are there tests that can be written for this new functionality? |
drewnoakes
left a comment
drewnoakes
left a comment
There was a problem hiding this comment.
Looks like a good addition, thanks. Left a few comments.
It'd be great to add a playground application that exercised these options, so there was an F5-able test bed, for manual testing.
There are integration tests for the current gRPC-based OTLP interactions. Having similar tests for the HTTP interactions would be valuable to ensure this scenario doesn't break.
I added for all endpoints IntegrationTests in |
There was a problem hiding this comment.
Some small nits but also big issues. I need to read the OTLP HTTP spec to understand what exactly we need to support.
I'm on leave at the moment, but when I get back (beginning of June) are you ok with me making changes to your PR? I think communicating everything via comments will be too much overhead.
dotnet-policy-service
bot
added
the
needs-author-action
|
@JamesNK It's a big honor to receive a review of Newtonsoft.Json's author. Thanks for all your work! |
dotnet-policy-service
bot
removed
the
needs-author-action
|
I made a bunch of changes.
TODO: There is still work to get the url flowing from the AppHost to the dashboard. I want to make it so that both endpoints are created if specified in app host launchSettings.json. The gRPC address is priorizited. But if the gRPC address is missing then OTLP/HTTP is used. |
|
I think everything is done and this is mergeable. Looking for reviews and approvals. A follow-up piece of work is changing the app host templates to include |
| public const string ProtobufContentType = "application/x-protobuf"; | ||
| public const string JsonContentType = "application/json"; | ||
|
|
||
| public static void ConfigureHttpOtlp(this IEndpointRouteBuilder endpoints) |
There was a problem hiding this comment.
nit:
| public static void ConfigureHttpOtlp(this IEndpointRouteBuilder endpoints) | |
| public static void MapOtlpEndpoints(this IEndpointRouteBuilder endpoints) |
Map* is the verb we use for adding routes. Configure is for options.
There was a problem hiding this comment.
I think I came up with the Map* verb back in .NET Core 3 😋
Co-authored-by: Drew Noakes <git@drewnoakes.com>
| public const string ProtobufContentType = "application/x-protobuf"; | ||
| public const string JsonContentType = "application/json"; | ||
|
|
||
| public static void MapHttpOtlpApi(this IEndpointRouteBuilder endpoints) |
There was a problem hiding this comment.
It's typically recommended that MapX implementations like this one return the IEndpoitRouteBuilder instance passed in in order to support chaining invocations onto the call (e.g. adding metadata via WithMetadta). Is there a particular reason to prohibit chaining on this API?
There was a problem hiding this comment.
This is only called in on place. It’s not a library, just part of an app. It’s less important in that context
| } | ||
| } | ||
|
|
||
| private sealed class OtlpResult<T> : IResult where T : IMessage |
There was a problem hiding this comment.
Is it helpful for this result type to implement IEndpointMetadataProvider so that information about the response produced from these endpoints can surface to documentation tools like OpenAPI?
There was a problem hiding this comment.
Right now, the dashboard doesn't produce OpenAPI. This can improve later if that changes.
|
Merged! Thanks for your work on this @sharpSteff. |
7 participants
Hi Aspire-Team,
Great work with the Aspire-Dashboard.
I'm currently looking for a replacement for current OTLP-Backend (https://github.com/SigNoz/signoz), and Aspire Dashboard seems to be a lightweight sulution.
However we are relying in the HTTP OTLP Receiver (#3688). Therefore i tried my best to add HTTP OTLP endpoint support and it works quite will in our usecase.
With this changes Aspire Dashboard can consume OTLP Grpc and http(s) simultaneously. Due to the http/protobuf I could reuse the existing grpc-otlps models and services
Microsoft Reviewers: Open in CodeFlow