feat: Add OLTP Exporter Option for Protocol #2321
Conversation
fix: removing blank line
|
@tomkerkhove :) |
|
The PR was only opened yesterday, please bear with me. |
| public string CollectorUri { get; set; } | ||
| //Todo: Headers |
There was a problem hiding this comment.
Do we need this? We might want to remove this
| @@ -391,7 +392,7 @@ public async Task RuntimeConfiguration_HasConfiguredCollectorUriInOpenTelemetryC | |||
| // Arrange | |||
There was a problem hiding this comment.
Let's use InlineData to inject 2 protocol types to make sure it works with different versions and not grpc which can be the default
| { | ||
| public class OpenTelemetryCollectorSinkConfiguration | ||
| { | ||
| public OtlpExportProtocol Protocol { get; set; } = default(OtlpExportProtocol); |
There was a problem hiding this comment.
What is the default? THinking of this again, we need to make sure we don't break existing users
There was a problem hiding this comment.
Thinking of it again, this is probably what we want:
| public OtlpExportProtocol Protocol { get; set; } = default(OtlpExportProtocol); | |
| public OtlpExportProtocol? Protocol { get; set; }; |
Then in the code we will check if it was specified. If it was not, use the current protocol.
| @@ -48,6 +72,11 @@ public ValidationResult Run() | |||
| errorMessages.Add($"Configured URI ({collectorUri}) for the OpenTelemetry Collector is not a valid URI."); | |||
| } | |||
| } | |||
| // Protocol Validation | |||
| if (!TryParseProtocol(collectorProtocol, out var parsedProtocol)) | |||
There was a problem hiding this comment.
Why do we need this? If the protocol would be bad, then it would not be present. I think we can drop this logic and just rely on the SDK
| @@ -13,6 +14,27 @@ namespace Promitor.Agents.Scraper.Validation.Steps.Sinks | |||
| public class OpenTelemetryCollectorMetricSinkValidationStep : ValidationStep, IValidationStep | |||
| { | |||
| private readonly IOptions<ScraperRuntimeConfiguration> _runtimeConfiguration; | |||
| public static bool TryParseProtocol(string value, out OtlpExportProtocol result) | |||
There was a problem hiding this comment.
As mentioned below, we don't need this
| @@ -115,6 +115,7 @@ private string BuildOpenApiDescription(IConfiguration configuration) | |||
| if (metricSinkConfiguration.OpenTelemetryCollector != null) | |||
| { | |||
| openApiDescriptionBuilder.AppendLine($"<li>OpenTelemetry Collector located on {metricSinkConfiguration.OpenTelemetryCollector.CollectorUri}</li>"); | |||
| openApiDescriptionBuilder.AppendLine($"<li>OpenTelemetry Collector Protocol selected: {metricSinkConfiguration.OpenTelemetryCollector.Protocol}</li>"); | |||
There was a problem hiding this comment.
As mentioned earlier, this should be part of message in line 117
| metricSinkAsciiTable.AddRow("OpenTelemetry Collector", $"Url: {otelConfiguration.CollectorUri}."); | ||
| metricSinkAsciiTable.AddRow("OpenTelemetry Collector", $"Protocol: {otelConfiguration.Protocol}."); |
| .AddOtlpExporter(options => | ||
| { | ||
| options.Endpoint = new Uri(otelConfiguration.CollectorUri); | ||
| options.Protocol = otelConfiguration.Protocol; |
There was a problem hiding this comment.
Only assign when configured
| options.Protocol = otelConfiguration.Protocol; | |
| if(otelConfiguration.Protocol != null) | |
| { | |
| options.Protocol = otelConfiguration.Protocol; | |
| } |
|
Let me know when you are done incorporating the feedback and I'll do another review - Thanks! |
|
@bunkrur Any idea if you can complete the last comments or not? |
|
I can, yeah just swallowed up recently. Will try by tomorrow |
|
Let me know how you are doing @bunkrur (no pressure) |
Fixes #
I wanted to clean some stuff up and remove an alien commit from a friend. I feel like this is a happy compromise.
Some issues I ran into in the last attempt were