Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Feat: redux of last PR with less chaos
  • Loading branch information
bunkrur committed Jun 13, 2023
commit cc5180ae1ad049017401da5f73cfa57080ad8fe1
1 change: 1 addition & 0 deletions config/promitor/scraper/runtime.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ metricSinks:
promitorMetricName: promitor_demo_documentation_availability
openTelemetryCollector:
collectorUri: https://opentelemetry-collector:4317
protocol: grpc
prometheusScrapingEndpoint:
metricUnavailableValue: -1
enableMetricTimestamps: true # true by default
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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}.");
Comment on lines +229 to +230
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be consolidated


var resourceBuilder = ResourceBuilder.CreateDefault()
.AddService(OpenTelemetryServiceName, serviceVersion: agentVersion);
Expand All @@ -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;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only assign when configured

Suggested change
options.Protocol = otelConfiguration.Protocol;
if(otelConfiguration.Protocol != null)
{
options.Protocol = otelConfiguration.Protocol;
}

}
);
});
services.AddTransient<IMetricSink, OpenTelemetryCollectorMetricSink>();
services.AddTransient<OpenTelemetryCollectorMetricSink>();
Expand Down
1 change: 1 addition & 0 deletions src/Promitor.Agents.Scraper/Startup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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>");
Copy link
Owner

Choose a reason for hiding this comment

The 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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned below, we don't need this

Copy link
Author

Choose a reason for hiding this comment

The 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)
Expand All @@ -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.");
Expand All @@ -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))
Copy link
Owner

Choose a reason for hiding this comment

The 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

Copy link
Author

Choose a reason for hiding this comment

The 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);
}
Expand Down
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);
Copy link
Owner

Choose a reason for hiding this comment

The 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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking of it again, this is probably what we want:

Suggested change
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.

public string CollectorUri { get; set; }
//Todo: Headers
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this? We might want to remove this

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -391,7 +392,7 @@ public async Task RuntimeConfiguration_HasConfiguredCollectorUriInOpenTelemetryC
// Arrange
Copy link
Owner

Choose a reason for hiding this comment

The 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
Expand All @@ -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]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System.Collections.Generic;
using Bogus;
using Microsoft.Extensions.Logging;
using OpenTelemetry.Exporter;
using Promitor.Agents.Core.Configuration.Server;
using Promitor.Agents.Core.Configuration.Telemetry;
using Promitor.Agents.Core.Configuration.Telemetry.Sinks;
Expand Down Expand Up @@ -156,6 +157,7 @@ private static OpenTelemetryCollectorSinkConfiguration GenerateOpenTelemetryColl
var openTelemetryCollectorSinkConfiguration = new Faker<OpenTelemetryCollectorSinkConfiguration>()
.StrictMode(true)
.RuleFor(promConfiguration => promConfiguration.CollectorUri, faker => faker.Internet.Url())
.RuleFor(promConfiguration => promConfiguration.Protocol, faker => OtlpExportProtocol.Grpc)
.Generate();
return openTelemetryCollectorSinkConfiguration;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Microsoft.Azure.Management.ResourceManager.Fluent.Core;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Logging;
using OpenTelemetry.Exporter;
using Promitor.Agents.Core.Configuration.Server;
using Promitor.Agents.Core.Configuration.Telemetry;
using Promitor.Agents.Core.Configuration.Telemetry.Sinks;
Expand Down Expand Up @@ -71,12 +72,12 @@ public RuntimeConfigurationGenerator WithPrometheusMetricSink(double? metricUnav

if (metricUnavailableValue != null)
{
prometheusSinkConfiguration.MetricUnavailableValue = (double) metricUnavailableValue;
prometheusSinkConfiguration.MetricUnavailableValue = (double)metricUnavailableValue;
}

if (enableMetricsTimestamp != null)
{
prometheusSinkConfiguration.EnableMetricTimestamps = (bool) enableMetricsTimestamp;
prometheusSinkConfiguration.EnableMetricTimestamps = (bool)enableMetricsTimestamp;
}
}

Expand All @@ -85,13 +86,14 @@ public RuntimeConfigurationGenerator WithPrometheusMetricSink(double? metricUnav
return this;
}

public RuntimeConfigurationGenerator WithOpenTelemetryCollectorMetricSink(string collectorUri = "https://opentelemetry-collector:8888")
public RuntimeConfigurationGenerator WithOpenTelemetryCollectorMetricSink(string collectorUri = "https://opentelemetry-collector:8888", OtlpExportProtocol protocol = OtlpExportProtocol.Grpc)
{
if (string.IsNullOrWhiteSpace(collectorUri) == false)
{
_runtimeConfiguration.MetricSinks.OpenTelemetryCollector = new OpenTelemetryCollectorSinkConfiguration
{
CollectorUri = collectorUri
CollectorUri = collectorUri,
Protocol = protocol
};
}

Expand Down Expand Up @@ -201,7 +203,7 @@ public RuntimeConfigurationGenerator WithContainerTelemetry(LogLevel? verbosity
};

_runtimeConfiguration.Telemetry ??= new TelemetryConfiguration();
_runtimeConfiguration.Telemetry.ContainerLogs = containerLogConfiguration;
_runtimeConfiguration.Telemetry.ContainerLogs = containerLogConfiguration;

return this;
}
Expand Down Expand Up @@ -296,6 +298,7 @@ public async Task<IConfiguration> GenerateAsync()
{
configurationBuilder.AppendLine(" openTelemetryCollector:");
configurationBuilder.AppendLine($" collectorUri: {_runtimeConfiguration?.MetricSinks.OpenTelemetryCollector.CollectorUri}");
configurationBuilder.AppendLine($" protocol: {_runtimeConfiguration?.MetricSinks.OpenTelemetryCollector.Protocol}");
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
using System.ComponentModel;
using System.Collections.Generic;
using System.ComponentModel;
using Bogus;
using Microsoft.Extensions.Logging.Abstractions;
using Microsoft.Extensions.Options;
using OpenTelemetry.Exporter;
using Promitor.Agents.Scraper.Configuration;
using Promitor.Agents.Scraper.Validation.Steps.Sinks;
using Promitor.Tests.Unit.Generators.Config;
Expand Down Expand Up @@ -54,6 +57,7 @@ public void Validate_NoSinksConfigured_Success()
PromitorAssert.ValidationIsSuccessful(validationResult);
}


[Theory]
[InlineData(null)]
[InlineData("")]
Expand All @@ -72,6 +76,50 @@ public void Validate_OpenTelemetryCollectorWithUnspecifiedCollectorUri_Fails(str
PromitorAssert.ValidationFailed(validationResult);
}

[Theory]
[InlineData("grpc")]
[InlineData("http")]
[InlineData("http/protobuf")]
[InlineData("httpprotobuf")]
public void Validate_OpenTelemetryCollectorWithValidProtocol_Success(string collectorProtocolString)
{
// Arrange
var runtimeConfiguration = CreateRuntimeConfiguration();

if (OpenTelemetryCollectorMetricSinkValidationStep.TryParseProtocol(collectorProtocolString, out var collectorProtocol))
{
runtimeConfiguration.Value.MetricSinks.OpenTelemetryCollector.Protocol = collectorProtocol;

// Act
var openTelemetryCollectorValidationStep = new OpenTelemetryCollectorMetricSinkValidationStep(runtimeConfiguration, NullLogger<OpenTelemetryCollectorMetricSinkValidationStep>.Instance);
var validationResult = openTelemetryCollectorValidationStep.Run();

// Assert
PromitorAssert.ValidationIsSuccessful(validationResult);
}
}

[Theory]
[InlineData("badprotocol")]
[InlineData("https")]
public void Validate_OpenTelemetryCollectorWithValidProtocol_FailsToDefault(string collectorProtocolString)
{
// Arrange
var runtimeConfiguration = CreateRuntimeConfiguration();

if (!OpenTelemetryCollectorMetricSinkValidationStep.TryParseProtocol(collectorProtocolString, out var collectorProtocol))
{
runtimeConfiguration.Value.MetricSinks.OpenTelemetryCollector.Protocol = collectorProtocol;

// Act
var openTelemetryCollectorValidationStep = new OpenTelemetryCollectorMetricSinkValidationStep(runtimeConfiguration, NullLogger<OpenTelemetryCollectorMetricSinkValidationStep>.Instance);
var validationResult = openTelemetryCollectorValidationStep.Run();

// Assert
PromitorAssert.ValidationIsSuccessful(validationResult);
}
}

[Theory]
[InlineData("https://")]
[InlineData("https://foo:bar")]
Expand Down