Add OpenTelemetry Collector extension/component #603

martinjt · 2025-03-26T17:35:34Z

**Closes #602 **

PR Checklist

Other information

martinjt · 2025-03-26T19:52:31Z

@dotnet-policy-service agree

martinjt · 2025-03-26T19:54:43Z

I'm not sure how ready this is yet. I've been playing with the API a bit to give it enough flexibility around the certificates, images and config files.

Feedback very welcome, I wanted to get this up as is before I added any tests and worked on the documentation, etc.

aaronpowell

Would it make sense that, rather than having a default collector image set, that it's a required property to be provided and there's a method that will set the defaults for you?

So, AddOpenTelemetryCollector is used if you want to specify a collector, whereas AddOpenTelemetryDefaultCollection uses the one that you have currently specified as a default.

aaronpowell · 2025-03-27T02:26:42Z

...Hosting.OpenTelemetryCollector/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector.csproj

+  <PropertyGroup>
+    <TargetFramework>net9.0</TargetFramework>
+    <ImplicitUsings>enable</ImplicitUsings>
+    <Nullable>enable</Nullable>
+  </PropertyGroup>
+


Suggested change

<PropertyGroup>

<TargetFramework>net9.0</TargetFramework>

<ImplicitUsings>enable</ImplicitUsings>

<Nullable>enable</Nullable>

</PropertyGroup>

This will all be provided by the props files above.

aaronpowell · 2025-03-27T02:28:20Z

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/EnvironmentVariableHook.cs

+    public EnvironmentVariableHook(ILogger<EnvironmentVariableHook> logger)
+    {
+        _logger = logger;
+    }


Use a primary constructor

aaronpowell · 2025-03-27T02:28:28Z

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/EnvironmentVariableHook.cs

+        var resources = appModel.GetProjectResources();
+        var collectorResource = appModel.Resources.OfType<CollectorResource>().FirstOrDefault();
+
+        if (collectorResource == null)


Suggested change

if (collectorResource == null)

if (collectorResource is null)

aaronpowell · 2025-03-27T02:28:49Z

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/EnvironmentVariableHook.cs

+            return Task.CompletedTask;
+        }
+
+        if (resources.Count() == 0)


Suggested change

if (resources.Count() == 0)

if (!resources.Any())

aaronpowell · 2025-03-27T02:28:59Z

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/EnvironmentVariableHook.cs

+        foreach (var resourceItem in resources)
+        {
+            _logger.LogDebug($"Forwarding Telemetry for {resourceItem.Name} to the collector");
+            if (resourceItem == null) continue;


Suggested change

if (resourceItem == null) continue;

if (resourceItem is null) continue;

aaronpowell · 2025-03-27T02:30:30Z

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/PublicAPI.Shipped.txt

File no longer needed

aaronpowell · 2025-03-27T02:30:39Z

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/PublicAPI.Unshipped.txt

File no longer needed

aaronpowell · 2025-03-27T02:31:53Z

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/DevCertHostingExtensions.cs

We can likely use this in a few places, it should help unblock #388 and #444, so could it go into the Shared folder and be added to the csproj.

aaronpowell · 2025-03-27T02:34:28Z

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/OpenTelemetryCollectorSettings.cs

+    /// <summary>
+    /// The version of the collector, defaults to latest
+    /// </summary>
+    public string CollectorVersion { get; set; } = "latest";


We should pin to a version so it's known stable per the package version

I'm not convinced that pinning is the right approach for that, especially since there isn't a v1 yet. It's generally accepted that people don't pin even in production.

aaronpowell · 2025-03-27T02:35:14Z

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/OpenTelemetryCollectorSettings.cs

+    /// <summary>
+    /// The image of the collector, defaults to ghcr.io/open-telemetry/opentelemetry-collector-releases/opentelemetry-collector-contrib
+    /// </summary>
+    public string CollectorImage { get; set; } = "ghcr.io/open-telemetry/opentelemetry-collector-releases/opentelemetry-collector-contrib";


I feel like this should be split to registry/image like we do with other container resources. And is there a reason you're using ghcr.io not Docker Hub?

ghcr is the recommended default since the limits went into dockerhub.

Do you have an example of the pattern other integrations use for the registry split.

Aspire/src/CommunityToolkit.Aspire.Hosting.Ollama/OllamaContainerImageTags.cs

Lines 5 to 6 in 0c0afa1

public const string Registry = "docker.io";

public const string Image = "ollama/ollama";

martinjt · 2025-03-27T07:55:54Z

Interesting approach, I'm honestly not sure, but maybe a .WithDefaultImage() might be better?

I was thinking that the approach I showed was more common, but happy if not.

I kinda wanted a minimal default command though.

aaronpowell · 2025-04-01T03:15:41Z

Interesting approach, I'm honestly not sure, but maybe a .WithDefaultImage() might be better?

I was thinking that the approach I showed was more common, but happy if not.

I kinda wanted a minimal default command though.

The problem with a method like WithDefaultImage is that you could setup one without any image specified which would be a runtime error, which would be a less than desirable outcome.

martinjt · 2025-04-09T18:42:34Z

I agree it's not ideal, but I'd prefer to not have them need to know what the image link is to collector. So I believe the best option is to use the default image we recommend in Otel right now (which is latest) when the extension is called without parameters.

martinjt · 2025-08-05T20:56:20Z

I'm coming back at this now @aaronpowell, with a hope to move it forward so I don't need to maintain a separate one (happy to be codeowner here).

On the devcerts, it seems those issues haven't got a resolution, would it be ok to leave the devcerts in this project for now, and consolidate/move later? This project would actually fix both of those issues as the users could run the non-ssl collector and forward their data through the collector as a way to unblock them.

The big outstanding issue is the collector image model, can you let me know what you'd suggest there. It should be latest since there isn't a v1 of the collector yet, and the advice from the collector team is to always use latest.

aaronpowell · 2025-08-06T02:23:29Z

On the devcerts, it seems those issues haven't got a resolution, would it be ok to leave the devcerts in this project for now, and consolidate/move later? This project would actually fix both of those issues as the users could run the non-ssl collector and forward their data through the collector as a way to unblock them.

If you can pop it in the Shared folder we can then add it as a linked file into several projects that could benefit from it in the CT as well.

The big outstanding issue is the collector image model, can you let me know what you'd suggest there. It should be latest since there isn't a v1 of the collector yet, and the advice from the collector team is to always use latest.

What's the release frequency that they are going with? The main reason that we pin to a version is so that we don't risk any unexpected changes to command line args or other behaviour getting introduced that impact someone who hasn't opt-ed in to using a newer version. If they are releasing new versions daily, then pinned isn't ideal, but generally speaking, pinning a version is going to be preferred, and people can always specify a version override if we don't release an updated package quick enough (Ollama is a good example of where we get caught on that front).

martinjt · 2025-08-06T09:19:26Z

The release cycle for the collector is every 2 weeks. The compatibility issues wouldn't be commandline args, its more likely the config of collector components that might change, but we haven't done a breaking change in about 6 months and those are expected to be the last.

The biggest issue with pinning a version is security issues, since the amount of components in the collector means that the updates contain those updates regularly.

I'll happily take guidance on what the best way to get this published is.

martinjt · 2025-08-07T21:33:54Z

I've done a few major updates, so it probably needs a full re-review.

Implemented inbound SSL, which can also be forced off (this helps fix some of the clients that can't use HTTPS because of the certificate issues)
Outbound SSL was removed, it won't work until the Certificate supports host.docker.internal or the *.localhost stuff works.
Moved to using .WithConfig("config.yaml"), I'm not sure how I'd fail startup if they don't provide that, I'm happy to implement it.
.WithConfig() supports multiple configs
Moved to using Random ports supplied by Aspire
Added SSL certificates for HTTP and gRPC
Made the Environment variables detect the protocol that the App wants and use that (useful as Node, by default, supports HTTP not gRPC)
Support for both of the endpoint environment variable names ASPIRE_DASHBOARD_OTLP_ENDPOINT_URL and DOTNET_DASHBOARD_OTLP_ENDPOINT_URL
Updated DevCerts to not try and add the certificates twice (this will only be useful when the Outbound SSL functionality is available.

This does require the user to implement insecure_skip_verify, but I can't find a way around that until the new certificate work to make the authority match.

Copilot

Pull Request Overview

This PR adds an OpenTelemetry Collector integration to the .NET Aspire Community Toolkit, enabling users to add an OpenTelemetry Collector container to their Aspire applications for observability data collection and forwarding.

Introduces a new hosting integration with configurable settings for the OpenTelemetry Collector
Provides automatic HTTPS certificate handling for secure connections in development mode
Implements environment variable hooks to automatically configure project resources to send telemetry data to the collector

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/Shared/DevCertHostingExtensions.cs	Shared utility for injecting ASP.NET Core HTTPS dev certificates into resources via environment variables
src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/OpenTelemetryCollectorSettings.cs	Configuration settings class for the OpenTelemetry Collector
src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/EnvironmentVariableHook.cs	Lifecycle hook to automatically configure project resources with collector endpoints
src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector.csproj	Project file for the OpenTelemetry Collector integration
src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/CollectorResource.cs	Resource definition for the OpenTelemetry Collector container
src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/CollectorExtensions.cs	Extension methods for adding and configuring the OpenTelemetry Collector
CommunityToolkit.Aspire.slnx	Solution file updated to include the new project

src/Shared/DevCertHostingExtensions.cs

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/EnvironmentVariableHook.cs

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/CollectorExtensions.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

davidfowl · 2025-08-10T02:05:18Z

src/Shared/DevCertHostingExtensions.cs

+    /// This method <strong>does not</strong> configure an HTTPS endpoint on the resource.
+    /// Use <see cref="ResourceBuilderExtensions.WithHttpsEndpoint{TResource}"/> to configure an HTTPS endpoint.
+    /// </remarks>
+    public static IResourceBuilder<TResource> RunWithHttpsDevCertificate<TResource>(


We don't ship this in any of the intergrations as yet and I think we'd rewrite it at this point to behave a bit better given the advances in the model.

Is there something "wrong" in the design of how this has been implemented that should be revised? Having a way to bootstrap the dev certificate into a container has been a pain point with a few integrations and having them working with OTEL (DAB and Go Feature Flags off the top of my head).

I can self-contain this inside of the Collector to reduce the impact? This was code taken from an example built by the Aspire team with a few amendments to allow some resilience.

It's internal, so it'll only be available in the assemblies it's packaged with, and I want to use it for others, so I'm find with where it is situated.

I'm more questioning @davidfowl's comment, as I'm not sure what he's looking for or suggesting.

aaronpowell

The main change here would be to use the event callbacks of 9.4.

Then would you be able to introduce an example project so I can get a better idea of how this works.

aaronpowell · 2025-08-11T01:19:26Z

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/CollectorExtensions.cs

+    /// <returns></returns>
+    public static IResourceBuilder<CollectorResource> WithAppForwarding(this IResourceBuilder<CollectorResource> builder)
+    {
+        builder.ApplicationBuilder.Services.TryAddLifecycleHook<EnvironmentVariableHook>();


Lifecycle hooks are mostly considered legacy with the event system now in place. The hook should be replacable with the OnAfterEndpointsAllocated callback.

I'm trying to gather from the documentation whether the OnAfterEndpointsAllocated is resource specific, or part of the wider DistributedAppModel. My concern being that this needs to be applied after the Aspire code has added the dashboard environment variables.

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/EnvironmentVariableHook.cs

aaronpowell · 2025-08-11T01:21:33Z

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/OpenTelemetryCollectorSettings.cs

+    /// <summary>
+    /// The image of the collector, defaults to ghcr.io/open-telemetry/opentelemetry-collector-releases/opentelemetry-collector-contrib
+    /// </summary>
+    public string CollectorImage { get; set; } = "ghcr.io/open-telemetry/opentelemetry-collector-releases/opentelemetry-collector-contrib";


Aspire/src/CommunityToolkit.Aspire.Hosting.Ollama/OllamaContainerImageTags.cs

Lines 5 to 6 in 0c0afa1

public const string Registry = "docker.io";

public const string Image = "ollama/ollama";

aaronpowell · 2025-08-11T01:23:54Z

src/Shared/DevCertHostingExtensions.cs

+    /// This method <strong>does not</strong> configure an HTTPS endpoint on the resource.
+    /// Use <see cref="ResourceBuilderExtensions.WithHttpsEndpoint{TResource}"/> to configure an HTTPS endpoint.
+    /// </remarks>
+    public static IResourceBuilder<TResource> RunWithHttpsDevCertificate<TResource>(


Is there something "wrong" in the design of how this has been implemented that should be revised? Having a way to bootstrap the dev certificate into a container has been a pain point with a few integrations and having them working with OTEL (DAB and Go Feature Flags off the top of my head).

src/Shared/DevCertHostingExtensions.cs

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/CollectorExtensions.cs

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/CollectorResource.cs

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/EnvironmentVariableHook.cs

martinjt · 2025-08-11T22:08:48Z

I've done everything suggested other than moving the EnvironmentVariableHook. Since this needs to occur after all the resources had endpoints allocated, I'm struggling to see how I might achieve that.

I did have a thought of using OnAfterEndpointsAllocatedEvent, but that's deprecated, and then thought of using OnResourceEndpointsAllocated event on the collector resource, then access each resource to add that same event to add the endpoint, but I'm not sure how to access the eventing model from there?

aaronpowell · 2025-08-11T23:41:09Z

Yes, sorry, I was mistaken, OnResourceEndpointsAllocated is the event that you'd want on the collector resource.

Is the logic that you'd have multiple collectors registered, or is it a singleton sort of thing?

martinjt · 2025-08-11T23:55:36Z

The collector is a service that intercepts the OTLP telemetry from all the other services, so it intercepts all the other project resources and overrides the OTLP environment variable to point it to the collector instead.

For that reason, I thought that using the resource focused events wasn't something I could do.

This is instead of telling the user to access every resource and reference it to the collector.

aaronpowell reviewed Mar 27, 2025

View reviewed changes

github-actions bot added the Stale label Apr 6, 2025

aaronpowell added awaiting response Waiting for the author of the issue to provide more information or answer a question and removed Stale labels Apr 8, 2025

martinjt force-pushed the otel-collector-component branch from 7ac9a9d to a38c078 Compare August 5, 2025 20:39

Added OpenTelemetry Collector component

7c26b63

martinjt force-pushed the otel-collector-component branch from 0ae0e03 to 7c26b63 Compare August 7, 2025 20:57

Fixed DevCertHostingExtensions

e6c40fe

martinjt marked this pull request as ready for review August 7, 2025 21:35

Copilot AI review requested due to automatic review settings August 7, 2025 21:35

Copilot AI reviewed Aug 7, 2025

View reviewed changes

martinjt and others added 2 commits August 7, 2025 22:41

Update src/Shared/DevCertHostingExtensions.cs

f347af6

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Apply suggestions from code review

aafae22

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

davidfowl removed the awaiting response Waiting for the author of the issue to provide more information or answer a question label Aug 9, 2025

davidfowl reviewed Aug 10, 2025

View reviewed changes

aaronpowell requested changes Aug 11, 2025

View reviewed changes

davidfowl reviewed Aug 11, 2025

View reviewed changes

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/CollectorExtensions.cs Outdated Show resolved Hide resolved

davidfowl reviewed Aug 11, 2025

View reviewed changes

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/CollectorResource.cs Outdated Show resolved Hide resolved

davidfowl reviewed Aug 11, 2025

View reviewed changes

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/CollectorResource.cs Outdated Show resolved Hide resolved

davidfowl reviewed Aug 11, 2025

View reviewed changes

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/CollectorResource.cs Outdated Show resolved Hide resolved

davidfowl reviewed Aug 11, 2025

View reviewed changes

src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/EnvironmentVariableHook.cs Show resolved Hide resolved

martinjt added 3 commits August 11, 2025 21:19

Renames

ef21d6b

Switch the image syntax

b3a8d93

Move to OnBeforeStart for DevCerts

fe14d9d

	if (collectorResource == null)
	if (collectorResource is null)

	if (resourceItem == null) continue;
	if (resourceItem is null) continue;

	public const string Registry = "docker.io";
	public const string Image = "ollama/ollama";

Add OpenTelemetry Collector extension/component #603

Are you sure you want to change the base?

Add OpenTelemetry Collector extension/component #603

Uh oh!

Conversation

martinjt commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Other information

Uh oh!

martinjt commented Mar 26, 2025

Uh oh!

martinjt commented Mar 26, 2025

Uh oh!

aaronpowell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinjt commented Mar 27, 2025

Uh oh!

aaronpowell commented Apr 1, 2025

Uh oh!

martinjt commented Apr 9, 2025

Uh oh!

martinjt commented Aug 5, 2025

Uh oh!

aaronpowell commented Aug 6, 2025

Uh oh!

martinjt commented Aug 6, 2025

Uh oh!

martinjt commented Aug 7, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronpowell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

martinjt commented Mar 26, 2025 •

edited

Loading