Add OpenTelemetry Collector extension/component #603
Conversation
|
@dotnet-policy-service agree |
|
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. |
There was a problem hiding this comment.
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.
| <PropertyGroup> | ||
| <TargetFramework>net9.0</TargetFramework> | ||
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <Nullable>enable</Nullable> | ||
| </PropertyGroup> | ||
|
|
There was a problem hiding this comment.
| <PropertyGroup> | |
| <TargetFramework>net9.0</TargetFramework> | |
| <ImplicitUsings>enable</ImplicitUsings> | |
| <Nullable>enable</Nullable> | |
| </PropertyGroup> |
This will all be provided by the props files above.
| public EnvironmentVariableHook(ILogger<EnvironmentVariableHook> logger) | ||
| { | ||
| _logger = logger; | ||
| } |
| var resources = appModel.GetProjectResources(); | ||
| var collectorResource = appModel.Resources.OfType<CollectorResource>().FirstOrDefault(); | ||
|
|
||
| if (collectorResource == null) |
There was a problem hiding this comment.
| if (collectorResource == null) | |
| if (collectorResource is null) |
| return Task.CompletedTask; | ||
| } | ||
|
|
||
| if (resources.Count() == 0) |
There was a problem hiding this comment.
| if (resources.Count() == 0) | |
| if (!resources.Any()) |
| foreach (var resourceItem in resources) | ||
| { | ||
| _logger.LogDebug($"Forwarding Telemetry for {resourceItem.Name} to the collector"); | ||
| if (resourceItem == null) continue; |
There was a problem hiding this comment.
| if (resourceItem == null) continue; | |
| if (resourceItem is null) continue; |
| /// <summary> | ||
| /// The version of the collector, defaults to latest | ||
| /// </summary> | ||
| public string CollectorVersion { get; set; } = "latest"; |
There was a problem hiding this comment.
We should pin to a version so it's known stable per the package version
There was a problem hiding this comment.
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.
| /// <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"; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Interesting approach, I'm honestly not sure, but maybe a 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 |
aaronpowell
added
awaiting response
|
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 |
**Closes #602 **
PR Checklist
Other information