Emit metrics via otel instead of custom format #4762

andreasohlund · 2025-01-31T13:17:04Z

This replaces the homegrown metrics in the audit instance with OTEL to allow for easier consumption in visualization tools like Prometheus, Azure Monitor etc

src/ServiceControl.Audit/Auditing/AuditPersister.cs

Co-authored-by: Ramon Smits <ramon.smits@gmail.com>

andreasohlund · 2025-02-03T07:39:40Z

@lailabougria can you spot anything off in terms of how we emit the telemetry? naming etc

ramonsmits · 2025-02-03T12:00:46Z

src/ServiceControl.Audit/Auditing/AuditIngestor.cs

-                new DetectSuccessfulRetriesEnricher(),
-                new SagaRelationshipsEnricher()
-            }.Concat(auditEnrichers).ToArray();
+            var enrichers = new IEnrichImportedAuditMessages[] { new MessageTypeEnricher(), new EnrichWithTrackingIds(), new ProcessingStatisticsEnricher(), new DetectNewEndpointsFromAuditImportsEnricher(endpointInstanceMonitoring), new DetectSuccessfulRetriesEnricher(), new SagaRelationshipsEnricher() }.Concat(auditEnrichers).ToArray();


@andreasohlund update your Rider formatting so that this will not be put on a single line

ramonsmits · 2025-02-03T21:50:12Z

src/ServiceControl.Audit/Auditing/AuditIngestion.cs

-        readonly Meter batchSizeMeter;
-        readonly Meter batchDurationMeter;
-        readonly Counter receivedMeter;
+        readonly Histogram<long> auditBatchSize = AuditMetrics.Meter.CreateHistogram<long>($"{AuditMetrics.Prefix}.batch_size_audits");


Why is this called batch_size_audits when the application is particular.servicecontrol.audit?

ramonsmits · 2025-02-03T21:52:45Z

src/ServiceControl.Audit/Auditing/AuditPersister.cs

+        readonly Counter<long> storedAudits = AuditMetrics.Meter.CreateCounter<long>($"{AuditMetrics.Prefix}.stored_audit_messages");
+        readonly Counter<long> storedSagas = AuditMetrics.Meter.CreateCounter<long>($"{AuditMetrics.Prefix}.stored_saga_audits");
+        readonly Histogram<double> auditBulkInsertDuration = AuditMetrics.Meter.CreateHistogram<double>($"{AuditMetrics.Prefix}.bulk_insert_duration_audits", unit: "ms");
+        readonly Histogram<double> sagaAuditBulkInsertDuration = AuditMetrics.Meter.CreateHistogram<double>($"{AuditMetrics.Prefix}.bulk_insert_duration_sagas", unit: "ms");
+        readonly Histogram<double> auditCommitDuration = AuditMetrics.Meter.CreateHistogram<double>($"{AuditMetrics.Prefix}.commit_duration_audits", unit: "ms");


I think we are better off managing all these metrics in one class with different methods for signaling a counter.

This way such method could also increase multiple counters or one instance counter and a lenght counters.

This also makes it much clearer what actual metrics exists.

Especially because here its also not clear what the name will be. I think its better if all names are listed in a single location including there prefix/scope.

ramonsmits · 2025-02-03T22:00:41Z

src/ServiceControl.Audit/HostApplicationBuilderExtensions.cs

+        {
+            if (!Uri.TryCreate(settings.OtelMetricsUrl, UriKind.Absolute, out var otelMetricsUri))
+            {
+                throw new UriFormatException($"Invalid OtelMetricsUrl: {settings.OtelMetricsUrl}");


This was returned in a search:

The environment variable OTEL_EXPORTER_OTLP_ENDPOINT exists as a standardized way to specify the OpenTelemetry OTLP endpoint. By default, it points to http://localhost:4317 for gRPC and http://localhost:4318 for HTTP/protobuf.
There are also some related environment variables:

OTEL_EXPORTER_OTLP_PROTOCOL for specifying the protocol (grpc, http/protobuf)
OTEL_EXPORTER_OTLP_HEADERS for additional headers
OTEL_EXPORTER_OTLP_TIMEOUT for request timeouts

Maybe instead we use a more verbose name?

OTEL_EXPORTER_OTLP_ENDPOINT

If not maybe immediately we should ensure that environment variable expansion is active so you could do:

<add key="ServiceControl.Audit/OTEL_EXPORTER_OTLP_ENDPOINT" value="%OTEL_EXPORTER_OTLP_ENDPOINT%" />

or for that matter:

SERVICECONTROL_AUDIT_OTEL_EXPORTER_OTLP_ENDPOINT=%OTEL_EXPORTER_OTLP_ENDPOINT%

ok, while testing envvar expansion behavior I stumbled upon the settings reader:

ServiceControl/src/ServiceControl.Configuration/EnvironmentVariableSettingsReader.cs

Lines 37 to 52 in ea0cb32

// The 3 app namespaces can be loaded from env vars without the namespace, so settings like TransportType can be

// shared between all instances in a container .env file. Settings in all other namespaces must be fully specified.

if (CanSetWithoutNamespaceList.Contains(settingsNamespace))

{

var nameOnly = regex.Replace(name, "_");

yield return nameOnly.ToUpperInvariant();

yield return nameOnly;

}

}

static readonly HashSet<SettingsRootNamespace> CanSetWithoutNamespaceList =

[

new SettingsRootNamespace("ServiceControl"),

new SettingsRootNamespace("ServiceControl.Audit"),

new SettingsRootNamespace("Monitoring")

];

This means we can't use it with a prefix as that logic would still load the value directly from OTEL_EXPORTER_OTLP_ENDPOINT.

ramonsmits · 2025-02-03T22:02:38Z

src/ServiceControl.Audit/HostApplicationBuilderExtensions.cs

+            }
+            builder.Services.AddOpenTelemetry()
+                .ConfigureResource(b => b.AddService(
+                    serviceName: "Particular.ServiceControl.Audit",


I think this needs to be added to AuditMetrics

ramonsmits · 2025-02-03T22:13:02Z

src/ServiceControl.Audit/Infrastructure/AuditMetrics.cs

+static class AuditMetrics
+{
+    public const string MeterName = "Particular.ServiceControl";
+    public static readonly Meter Meter = new(MeterName, "0.1.0");
+    public static readonly string Prefix = "particular.servicecontrol.audit";
+}


I think this should better define its intend:

No need for Audit in the name as the namespace already is clear

Its for OpenTelemetry

These are constants

Suggested change

static class AuditMetrics

{

public const string MeterName = "Particular.ServiceControl";

public static readonly Meter Meter = new(MeterName, "0.1.0");

public static readonly string Prefix = "particular.servicecontrol.audit";

}

static class TelemetryGlobals

{

public const string Source = "Particular.ServiceControl";

public const string Scope = "particular.servicecontrol.audit";

public static readonly Meter Meter = new(MeterName, "0.1.0");

}

ramonsmits · 2025-02-03T22:14:36Z

src/ServiceControl.Audit/Infrastructure/Settings/Settings.cs

@@ -109,6 +109,7 @@ public string RootUrl
        public int Port { get; set; }

        public bool PrintMetrics => SettingsReader.Read<bool>(SettingsRootNamespace, "PrintMetrics");
+        public string OtelMetricsUrl { get; set; } = SettingsReader.Read<string>(SettingsRootNamespace, nameof(OtelMetricsUrl));


See other comment on OTEL_EXPORTER_OTLP_ENDPOINT

Suggested change

public string OtelMetricsUrl { get; set; } = SettingsReader.Read<string>(SettingsRootNamespace, nameof(OtelMetricsUrl));

const string OTEL_EXPORTER_OTLP_ENDPOINT = nameof(OTEL_EXPORTER_OTLP_ENDPOINT);

[]

public string OtelExporterOltpEndpointUrl { get; init; } = SettingsReader.Read<string>(SettingsRootNamespace, OTEL_EXPORTER_OTLP_ENDPOINT);

ramonsmits · 2025-02-03T22:27:30Z

src/ServiceControl.Audit/Auditing/AuditPersister.cs

+                        var auditSw = Stopwatch.StartNew();
+                        await unitOfWork.RecordProcessedMessage(processedMessage, context.Body);
+                        auditBulkInsertDuration.Record(auditSw.ElapsedMilliseconds);


There are a lot of things done in this method. Even multiple measurements.
I think we should consider doing the measures in the unitOfWork methods itself. The measurement is then simply that whole method and isn't making this method even worse then it already is :-)

ramonsmits · 2025-02-03T22:53:23Z

src/ServiceControl.Audit/Auditing/AuditIngestion.cs

+        readonly Histogram<long> auditBatchSize = AuditMetrics.Meter.CreateHistogram<long>($"{AuditMetrics.Prefix}.batch_size_audits");
+        readonly Histogram<double> auditBatchDuration = AuditMetrics.Meter.CreateHistogram<double>($"{AuditMetrics.Prefix}.batch_duration_audits", unit: "ms");
+        readonly Histogram<double> auditMessageSize = AuditMetrics.Meter.CreateHistogram<double>($"{AuditMetrics.Prefix}.audit_message_size", unit: "kilobytes");
+        readonly Counter<long> receivedAudits = AuditMetrics.Meter.CreateCounter<long>($"{AuditMetrics.Prefix}.received_audits");


There is something wrong with the naming here. THese shouldn't have a prefix like this:

Suggested change

readonly Counter<long> receivedAudits = AuditMetrics.Meter.CreateCounter<long>($"{AuditMetrics.Prefix}.received_audits");

readonly Counter<long> receivedAudits = AuditMetrics.Meter.CreateCounter<long>(

"received_total",

description: "Total number of audit messages received"

);

ramonsmits · 2025-02-03T23:03:10Z

src/ServiceControl.Audit/Auditing/AuditPersister.cs

-        readonly Meter bulkInsertCommitDurationMeter;
-        readonly IMessageSession messageSession;
-        readonly Lazy<IMessageDispatcher> messageDispatcher;
+        readonly Counter<long> storedAudits = AuditMetrics.Meter.CreateCounter<long>($"{AuditMetrics.Prefix}.stored_audit_messages");


Same here, these prefixes shouldn't be used and isn't according to spec. The Meter should define the prefix/scope AFAIK:

https://learn.microsoft.com/en-us/dotnet/core/diagnostics/metrics-instrumentation

var meter = meterFactory.Create("Particular.ServiceControl.Audit"); stored_messages_total = meter.CreateCounter<int>("stored_messages_total");

or

var meter = meterFactory.Create("Particular.ServiceControl"); stored_messages_total = meter.CreateCounter<int>("audit.stored_messages_total");

but as we have seperate instance my preference would be the 1st.

andreasohlund self-assigned this Jan 31, 2025

andreasohlund requested a review from ramonsmits January 31, 2025 13:17

ramonsmits reviewed Jan 31, 2025

View reviewed changes

src/ServiceControl.Audit/Auditing/AuditPersister.cs Outdated Show resolved Hide resolved

ramonsmits reviewed Jan 31, 2025

View reviewed changes

src/ServiceControl.Audit/Auditing/AuditPersister.cs Outdated Show resolved Hide resolved

ramonsmits reviewed Jan 31, 2025

View reviewed changes

src/ServiceControl.Audit/Auditing/AuditPersister.cs Outdated Show resolved Hide resolved

andreasohlund and others added 6 commits February 2, 2025 17:23

Emit metrics via otel instead of custom format

eb6e9dc

Minimize diff by reverting whitespace changes

2d0880b

Approvals

9652aa0

Fix formatting

77b2c2c

Apply suggestions from code review

03690a0

Co-authored-by: Ramon Smits <ramon.smits@gmail.com>

fix formatting

14c3474

andreasohlund force-pushed the otel-metrics branch from b986142 to 14c3474 Compare February 2, 2025 16:23

Only use standar otel

7f37e53

andreasohlund requested a review from ramonsmits February 2, 2025 18:51

andreasohlund added 3 commits February 2, 2025 20:05

Set instance id

7616151

Set unit

e6d1f40

Better metrics names

88d2e71

andreasohlund requested a review from lailabougria February 3, 2025 07:38

Emit body size

da4c35b

ramonsmits reviewed Feb 3, 2025

View reviewed changes

ramonsmits added 2 commits February 3, 2025 16:09

Fix meter name

786559b

Log that OpenTelemetry metrics exporter is enabled

a2011fc

ramonsmits requested changes Feb 3, 2025

View reviewed changes

ramonsmits reviewed Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit metrics via otel instead of custom format #4762

Emit metrics via otel instead of custom format #4762

andreasohlund commented Jan 31, 2025 •

edited

Loading

andreasohlund commented Feb 3, 2025

ramonsmits Feb 3, 2025

ramonsmits Feb 3, 2025

ramonsmits Feb 3, 2025

ramonsmits Feb 3, 2025

ramonsmits Feb 3, 2025

ramonsmits Feb 4, 2025

ramonsmits Feb 3, 2025

ramonsmits Feb 3, 2025

ramonsmits Feb 3, 2025

ramonsmits Feb 3, 2025

ramonsmits Feb 3, 2025

ramonsmits Feb 3, 2025 •

edited

Loading

	// The 3 app namespaces can be loaded from env vars without the namespace, so settings like TransportType can be
	// shared between all instances in a container .env file. Settings in all other namespaces must be fully specified.
	if (CanSetWithoutNamespaceList.Contains(settingsNamespace))
	{
	var nameOnly = regex.Replace(name, "_");
	yield return nameOnly.ToUpperInvariant();
	yield return nameOnly;
	}
	}

	static readonly HashSet<SettingsRootNamespace> CanSetWithoutNamespaceList =
	[
	new SettingsRootNamespace("ServiceControl"),
	new SettingsRootNamespace("ServiceControl.Audit"),
	new SettingsRootNamespace("Monitoring")
	];

-        public string OtelMetricsUrl { get; set; } = SettingsReader.Read<string>(SettingsRootNamespace, nameof(OtelMetricsUrl));
+        const string OTEL_EXPORTER_OTLP_ENDPOINT = nameof(OTEL_EXPORTER_OTLP_ENDPOINT);
+        []
+        public string OtelExporterOltpEndpointUrl { get; init; } = SettingsReader.Read<string>(SettingsRootNamespace, OTEL_EXPORTER_OTLP_ENDPOINT);

Emit metrics via otel instead of custom format #4762

Are you sure you want to change the base?

Emit metrics via otel instead of custom format #4762

Conversation

andreasohlund commented Jan 31, 2025 • edited Loading

andreasohlund commented Feb 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonsmits Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

andreasohlund commented Jan 31, 2025 •

edited

Loading

ramonsmits Feb 3, 2025 •

edited

Loading