-
-
Notifications
You must be signed in to change notification settings - Fork 94
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
feat: Add OLTP Exporter Option for Protocol #2321
base: master
Are you sure you want to change the base?
Changes from 3 commits
cc5180a
053fd48
a382b3e
ed6f037
d61a158
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -198,7 +198,7 @@ public static IServiceCollection UseMetricSinks(this IServiceCollection services | |||||||||||
if (metricSinkConfiguration?.OpenTelemetryCollector != null | ||||||||||||
&& string.IsNullOrWhiteSpace(metricSinkConfiguration.OpenTelemetryCollector.CollectorUri) == false) | ||||||||||||
{ | ||||||||||||
AddOpenTelemetryCollectorMetricSink(metricSinkConfiguration.OpenTelemetryCollector.CollectorUri, agentVersion, services, metricSinkAsciiTable); | ||||||||||||
AddOpenTelemetryCollectorMetricSink(metricSinkConfiguration.OpenTelemetryCollector, agentVersion, services, metricSinkAsciiTable); | ||||||||||||
} | ||||||||||||
|
||||||||||||
AnsiConsole.Write(metricSinkAsciiTable); | ||||||||||||
|
@@ -224,9 +224,10 @@ private static void AddAtlassianStatuspageMetricSink(string pageId, IServiceColl | |||||||||||
|
||||||||||||
const string OpenTelemetryServiceName = "promitor-scraper"; | ||||||||||||
|
||||||||||||
private static void AddOpenTelemetryCollectorMetricSink(string collectorUri, string agentVersion, IServiceCollection services, Table metricSinkAsciiTable) | ||||||||||||
private static void AddOpenTelemetryCollectorMetricSink(Promitor.Integrations.Sinks.OpenTelemetry.Configuration.OpenTelemetryCollectorSinkConfiguration otelConfiguration, string agentVersion, IServiceCollection services, Table metricSinkAsciiTable) | ||||||||||||
{ | ||||||||||||
metricSinkAsciiTable.AddRow("OpenTelemetry Collector", $"Url: {collectorUri}."); | ||||||||||||
metricSinkAsciiTable.AddRow("OpenTelemetry Collector", $"Url: {otelConfiguration.CollectorUri}."); | ||||||||||||
metricSinkAsciiTable.AddRow("OpenTelemetry Collector", $"Protocol: {otelConfiguration.Protocol}."); | ||||||||||||
|
||||||||||||
var resourceBuilder = ResourceBuilder.CreateDefault() | ||||||||||||
.AddService(OpenTelemetryServiceName, serviceVersion: agentVersion); | ||||||||||||
|
@@ -236,7 +237,12 @@ private static void AddOpenTelemetryCollectorMetricSink(string collectorUri, str | |||||||||||
{ | ||||||||||||
metricsBuilder.SetResourceBuilder(resourceBuilder) | ||||||||||||
.AddMeter("Promitor.Scraper.Metrics.AzureMonitor") | ||||||||||||
.AddOtlpExporter(options => options.Endpoint = new Uri(collectorUri)); | ||||||||||||
.AddOtlpExporter(options => | ||||||||||||
{ | ||||||||||||
options.Endpoint = new Uri(otelConfiguration.CollectorUri); | ||||||||||||
options.Protocol = otelConfiguration.Protocol; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only assign when configured
Suggested change
|
||||||||||||
} | ||||||||||||
); | ||||||||||||
}); | ||||||||||||
services.AddTransient<IMetricSink, OpenTelemetryCollectorMetricSink>(); | ||||||||||||
services.AddTransient<OpenTelemetryCollectorMetricSink>(); | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned earlier, this should be part of message in line 117 |
||
} | ||
|
||
if (metricSinkConfiguration.Statsd != null) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
using System.Linq; | ||
using Microsoft.Extensions.Logging; | ||
using Microsoft.Extensions.Options; | ||
using OpenTelemetry.Exporter; | ||
using Promitor.Agents.Core.Validation; | ||
using Promitor.Agents.Core.Validation.Interfaces; | ||
using Promitor.Agents.Core.Validation.Steps; | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned below, we don't need this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removing |
||
{ | ||
switch (value?.Trim().ToLower()) | ||
{ | ||
case "grpc": | ||
result = OtlpExportProtocol.Grpc; | ||
return true; | ||
case "http/protobuf": | ||
result = OtlpExportProtocol.HttpProtobuf; | ||
return true; | ||
case "httpprotobuf": | ||
result = OtlpExportProtocol.HttpProtobuf; | ||
return true; | ||
case "http": | ||
result = OtlpExportProtocol.HttpProtobuf; | ||
return true; | ||
default: | ||
result = default; | ||
return false; | ||
} | ||
} | ||
|
||
public OpenTelemetryCollectorMetricSinkValidationStep(IOptions<ScraperRuntimeConfiguration> runtimeConfiguration, ILogger<OpenTelemetryCollectorMetricSinkValidationStep> validationLogger) | ||
: base(validationLogger) | ||
|
@@ -33,6 +55,8 @@ public ValidationResult Run() | |
|
||
var errorMessages = new List<string>(); | ||
var collectorUri = openTelemetryCollectorSinkConfiguration.CollectorUri; | ||
var collectorProtocol = openTelemetryCollectorSinkConfiguration.Protocol.ToString(); | ||
// Url Validation | ||
if (string.IsNullOrWhiteSpace(collectorUri)) | ||
{ | ||
errorMessages.Add("No URI for the OpenTelemetry Collector is configured."); | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing |
||
{ | ||
errorMessages.Add($"Invalid Protocol ({collectorProtocol}) for the OpenTelemetry Collector is configured. Please check here for valid protocols: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/README.md#otlpexporteroptions"); | ||
} | ||
|
||
return errorMessages.Any() ? ValidationResult.Failure(ComponentName, errorMessages) : ValidationResult.Successful(ComponentName); | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,7 +1,11 @@ | ||||||
namespace Promitor.Integrations.Sinks.OpenTelemetry.Configuration | ||||||
using OpenTelemetry.Exporter; | ||||||
|
||||||
namespace Promitor.Integrations.Sinks.OpenTelemetry.Configuration | ||||||
{ | ||||||
public class OpenTelemetryCollectorSinkConfiguration | ||||||
{ | ||||||
public OtlpExportProtocol Protocol { get; set; } = default(OtlpExportProtocol); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking of it again, this is probably what we want:
Suggested change
Then in the code we will check if it was specified. If it was not, use the current protocol. |
||||||
public string CollectorUri { get; set; } | ||||||
//Todo: Headers | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this? We might want to remove this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removing |
||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
using Microsoft.Azure.Management.ResourceManager.Fluent.Core; | ||
using Microsoft.Extensions.Configuration; | ||
using Microsoft.Extensions.Logging; | ||
using OpenTelemetry.Exporter; | ||
using Promitor.Agents.Scraper.Configuration; | ||
using Promitor.Integrations.Sinks.Statsd.Configuration; | ||
using Promitor.Tests.Unit.Generators.Config; | ||
|
@@ -391,7 +392,7 @@ public async Task RuntimeConfiguration_HasConfiguredCollectorUriInOpenTelemetryC | |
// Arrange | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
var collectorUri = "https://foo.bar"; | ||
var configuration = await RuntimeConfigurationGenerator.WithServerConfiguration() | ||
.WithOpenTelemetryCollectorMetricSink(collectorUri: collectorUri) | ||
.WithOpenTelemetryCollectorMetricSink(collectorUri: collectorUri, protocol: OtlpExportProtocol.Grpc) | ||
.GenerateAsync(); | ||
|
||
// Act | ||
|
@@ -402,6 +403,7 @@ public async Task RuntimeConfiguration_HasConfiguredCollectorUriInOpenTelemetryC | |
Assert.NotNull(runtimeConfiguration.MetricSinks); | ||
Assert.NotNull(runtimeConfiguration.MetricSinks.OpenTelemetryCollector); | ||
Assert.Equal(collectorUri, runtimeConfiguration.MetricSinks.OpenTelemetryCollector.CollectorUri); | ||
Assert.Equal(OtlpExportProtocol.Grpc, runtimeConfiguration.MetricSinks.OpenTelemetryCollector.Protocol); | ||
} | ||
|
||
[Fact] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be consolidated