Convert Seq component to use OTEL's `AddProcessor` instead of `AddOtlpExporter` #3697

liammclennan · 2024-04-15T10:06:48Z

OpenTelemetry's AddOtlpExporter now throws an exception, so the Seq component is not working.

This PR switches to AddProcessor and adds some configuration work to compensate for AddProcessor not picking up .NET's OTEL configuration (as described by @CodeBlanch here). Adding OtlpExporterOptions onto Seq's configuration makes it possible to set the most vital options.

Changes due to #3546

Thank you to @CodeBlanch for showing the way forward!

Microsoft Reviewers: Open in CodeFlow

…Processor`.

…b.com/liammclennan/aspire into seq-component-switch-to-add-processor

…processor Seq component switch to add processor

eerhardt · 2024-04-15T14:05:43Z

src/Components/Aspire.Seq/SeqSettings.cs

+    public OtlpExporterOptions Logs
+    {
+        get;
+        private set;
+    } = new ();


Suggested change

public OtlpExporterOptions Logs

{

get;

private set;

} = new ();

public OtlpExporterOptions Logs { get; } = new();

No need for a private setter, it is never used.

eerhardt · 2024-04-15T14:06:04Z

src/Components/Aspire.Seq/SeqSettings.cs

+    public OtlpExporterOptions Traces
+    {
+        get;
+        private set;
+    } = new ();
+


Suggested change

public OtlpExporterOptions Traces

{

get;

private set;

} = new ();

public OtlpExporterOptions Traces { get; } = new();

eerhardt · 2024-04-15T14:16:15Z

src/Components/Aspire.Seq/AspireSeqExtensions.cs

            ));

-        if (settings.HealthChecks)
+        if (settings is { HealthChecks: true, ServerUrl: not null })


Should it be an error if you haven't set ServerUrl? Seems odd that setting HealthChecks = true, but not getting health checks would be silent.

I don't think it should be an error because a user could set Logs.EndPoint or Traces.EndPoint directly.

The combination of no ServerUrl and HealthChecks == true is never valid, so I will make that an error.

eerhardt · 2024-04-15T14:20:06Z

src/Components/Aspire.Seq/AspireSeqExtensions.cs

+        settings.ServerUrl = builder.Configuration.GetConnectionString(connectionName);
+        settings.Logs.Protocol = OtlpExportProtocol.HttpProtobuf;
+        settings.Traces.Protocol = OtlpExportProtocol.HttpProtobuf;
+
        builder.Configuration.GetSection("Aspire:Seq").Bind(settings);


The settings should continue to be bound to the configuration section before getting the connection string from connectionName. That is consistent with every other component. The order of precedence goes (lower number wins):

User code (configureSettings) always wins

ConnectionStrings w/ connection name

Configuration section

Thanks. Done.

CodeBlanch · 2024-04-15T16:20:00Z

src/Components/Aspire.Seq/AspireSeqExtensions.cs

-                    }
-                }
+        builder.Services.Configure<OpenTelemetryLoggerOptions>(logging => logging.AddProcessor(
+            sp => new BatchLogRecordExportProcessor(new OtlpLogExporter(settings.Logs))


@liammclennan If you want to respect ExportProcessorType make this...

builder.Services.Configure<OpenTelemetryLoggerOptions>(logging => logging.AddProcessor( sp => settings.Logs.ExportProcessorType == ExportProcessorType.Batch ? new BatchLogRecordExportProcessor(new OtlpLogExporter(settings.Logs)) : new SimpleLogRecordExportProcessor(new OtlpLogExporter(settings.Logs)) ));

Similar for below in tracing.

Thanks for the review. I've made this change.

…nabled.

Throw an exception if ServerUrl is unspecified, but HealthChecks is e…

Fix tests

JamesNK · 2024-04-16T00:27:32Z

@reyang @davidfowl UseOtlpExporter seems to be causing problems when integrating with other OTEL APIs. Is this intended? Won't this make it more likely that libraries with OTEL configuration will throw errors when used in an Aspire app?

davidfowl · 2024-04-16T05:25:53Z

src/Components/Aspire.Seq/AspireSeqExtensions.cs

+            _ => settings.Logs.ExportProcessorType == ExportProcessorType.Batch
+                ? new BatchLogRecordExportProcessor(new OtlpLogExporter(settings.Logs))
+                : new SimpleLogRecordExportProcessor(new OtlpLogExporter(settings.Logs))


switch expressions are much cleaner here:

Suggested change

_ => settings.Logs.ExportProcessorType == ExportProcessorType.Batch

? new BatchLogRecordExportProcessor(new OtlpLogExporter(settings.Logs))

: new SimpleLogRecordExportProcessor(new OtlpLogExporter(settings.Logs))

_ => settings.Logs.ExportProcessorType switch

ExportProcessorType.Batch => new BatchLogRecordExportProcessor(new OtlpLogExporter(settings.Logs)),

_ => new SimpleLogRecordExportProcessor(new OtlpLogExporter(settings.Logs))

Thanks @davidfowl

This change is done.

davidfowl · 2024-04-16T05:26:23Z

src/Components/Aspire.Seq/SeqSettings.cs

+    public OtlpExporterOptions Logs
+    {
+        get;
+    } = new ();


This formatting is very weird:

Suggested change

public OtlpExporterOptions Logs

{

get;

} = new ();

public OtlpExporterOptions Logs { get; } = new();

davidfowl · 2024-04-16T05:26:50Z

src/Components/Aspire.Seq/SeqSettings.cs

+    public OtlpExporterOptions Traces
+    {
+        get;
+    } = new ();
+


Suggested change

public OtlpExporterOptions Traces

{

get;

} = new ();

public OtlpExporterOptions Traces { get; } = new();

PR feedback

eerhardt

LGTM. Thanks for the fix!

eerhardt · 2024-04-16T16:24:59Z

/backport to release/8.0

github-actions · 2024-04-16T16:25:10Z

Started backporting to release/8.0: https://github.com/dotnet/aspire/actions/runs/8709142034

github-actions · 2024-04-16T16:25:13Z

@eerhardt backporting to release/8.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Switch Seq OpenTelemetry configuration from `AddOtlpExporter` to `AddProcessor`.
Using index info to reconstruct a base tree...
M	playground/seq/Seq.AppHost/aspire-manifest.json
A	src/Components/Aspire.Seq/PublicAPI.Unshipped.txt
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): src/Components/Aspire.Seq/PublicAPI.Unshipped.txt deleted in HEAD and modified in Switch Seq OpenTelemetry configuration from `AddOtlpExporter` to `AddProcessor`.. Version Switch Seq OpenTelemetry configuration from `AddOtlpExporter` to `AddProcessor`. of src/Components/Aspire.Seq/PublicAPI.Unshipped.txt left in tree.
Auto-merging playground/seq/Seq.AppHost/aspire-manifest.json
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Switch Seq OpenTelemetry configuration from `AddOtlpExporter` to `AddProcessor`.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2024-04-16T16:25:14Z

@eerhardt an error occurred while backporting to release/8.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

…pExporter` (dotnet#3697) * Switch Seq OpenTelemetry configuration from `AddOtlpExporter` to `AddProcessor`. * Switch Seq OpenTelemetry configuration from `AddOtlpExporter` to `AddProcessor`. * Move OTLP exporter options onto SeqSettings.cs * Add test * Update README * Renamed Seq settings * Change order of config * Updated code doc * ConfigurationSchema.json * Throw an exception if ServerUrl is unspecified, but HealthChecks is enabled. * Fix tests * PR feedback Fix dotnet#3546

…pExporter` (#3697) (#3728) * Switch Seq OpenTelemetry configuration from `AddOtlpExporter` to `AddProcessor`. * Switch Seq OpenTelemetry configuration from `AddOtlpExporter` to `AddProcessor`. * Move OTLP exporter options onto SeqSettings.cs * Add test * Update README * Renamed Seq settings * Change order of config * Updated code doc * ConfigurationSchema.json * Throw an exception if ServerUrl is unspecified, but HealthChecks is enabled. * Fix tests * PR feedback Fix #3546 Co-authored-by: Liam McLennan <lmclennan@datalust.co>

liammclennan added 11 commits April 15, 2024 10:11

Switch Seq OpenTelemetry configuration from AddOtlpExporter to `Add…

2e7e69e

…Processor`.

Switch Seq OpenTelemetry configuration from AddOtlpExporter to `Add…

f608beb

…Processor`.

Merge branch 'seq-component-switch-to-add-processor' of https://githu…

8278ebc

…b.com/liammclennan/aspire into seq-component-switch-to-add-processor

Move OTLP exporter options onto SeqSettings.cs

9c926c8

Add test

892893a

Update README

b4f877a

Renamed Seq settings

df2a586

Change order of config

e7e47f0

Updated code doc

bf7a896

ConfigurationSchema.json

478d63f

Merge pull request #20 from liammclennan/seq-component-switch-to-add-…

9c50681

…processor Seq component switch to add processor

dotnet-issue-labeler bot added the area-integrations Issues pertaining to Aspire Integrations packages label Apr 15, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 15, 2024

eerhardt reviewed Apr 15, 2024

View reviewed changes

CodeBlanch reviewed Apr 15, 2024

View reviewed changes

liammclennan and others added 4 commits April 16, 2024 10:03

Throw an exception if ServerUrl is unspecified, but HealthChecks is e…

5d32511

…nabled.

Merge pull request #21 from liammclennan/seq-pr-feedback

9c9505a

Throw an exception if ServerUrl is unspecified, but HealthChecks is e…

Fix tests

415fbd7

Merge pull request #22 from liammclennan/seq-pr-feedback

59193cb

Fix tests

davidfowl reviewed Apr 16, 2024

View reviewed changes

liammclennan and others added 2 commits April 16, 2024 16:10

PR feedback

54b5630

Merge pull request #24 from liammclennan/seq-pr-feedback-deux

59c5eae

PR feedback

eerhardt approved these changes Apr 16, 2024

View reviewed changes

eerhardt merged commit e58ae7d into dotnet:main Apr 16, 2024
8 checks passed

eerhardt mentioned this pull request Apr 16, 2024

[release/8.0] Seq component is broken with latest UseOtlpExporter - Convert Seq component to use OTEL's AddProcessor instead of AddOtlpExporter (#3697) #3728

Merged

CodeBlanch mentioned this pull request Apr 16, 2024

[otlp] UseOtlpExporter and AddOtlpExporter extensions don't play nice together open-telemetry/opentelemetry-dotnet#5538

Open

github-actions bot locked and limited conversation to collaborators May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Seq component to use OTEL's `AddProcessor` instead of `AddOtlpExporter` #3697

Convert Seq component to use OTEL's `AddProcessor` instead of `AddOtlpExporter` #3697

liammclennan commented Apr 15, 2024 •

edited by dotnet-policy-service bot

Loading

eerhardt Apr 15, 2024

liammclennan Apr 16, 2024

eerhardt Apr 15, 2024

liammclennan Apr 16, 2024

eerhardt Apr 15, 2024

liammclennan Apr 15, 2024

eerhardt Apr 15, 2024

liammclennan Apr 16, 2024

CodeBlanch Apr 15, 2024

liammclennan Apr 16, 2024

JamesNK commented Apr 16, 2024

davidfowl Apr 16, 2024

liammclennan Apr 16, 2024

davidfowl Apr 16, 2024 •

edited

Loading

liammclennan Apr 16, 2024

davidfowl Apr 16, 2024

liammclennan Apr 16, 2024

eerhardt left a comment

eerhardt commented Apr 16, 2024

github-actions bot commented Apr 16, 2024

github-actions bot commented Apr 16, 2024

github-actions bot commented Apr 16, 2024

Convert Seq component to use OTEL's AddProcessor instead of AddOtlpExporter #3697

Convert Seq component to use OTEL's AddProcessor instead of AddOtlpExporter #3697

Conversation

liammclennan commented Apr 15, 2024 • edited by dotnet-policy-service bot Loading

Microsoft Reviewers: Open in CodeFlow

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

JamesNK commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Apr 16, 2024 • edited Loading

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

eerhardt left a comment

Choose a reason for hiding this comment

eerhardt commented Apr 16, 2024

github-actions bot commented Apr 16, 2024

github-actions bot commented Apr 16, 2024

github-actions bot commented Apr 16, 2024

Convert Seq component to use OTEL's `AddProcessor` instead of `AddOtlpExporter` #3697

Convert Seq component to use OTEL's `AddProcessor` instead of `AddOtlpExporter` #3697

liammclennan commented Apr 15, 2024 •

edited by dotnet-policy-service bot

Loading

davidfowl Apr 16, 2024 •

edited

Loading