Skip to content
Merged
Show file tree
Hide file tree
Changes from 16 commits
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
1 change: 1 addition & 0 deletions src/OpenTelemetry.Exporter.Geneva/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

* Add ResourceFieldNames to filter resource attributes to send to Geneva
([#3552](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/3552))
([#3646](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/3646))

## 1.14.0

Expand Down
2 changes: 2 additions & 0 deletions src/OpenTelemetry.Exporter.Geneva/GenevaExporterOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ public IReadOnlyDictionary<string, string>? TableNameMappings

/// <summary>
/// Gets or sets prepopulated fields.
///
/// Pre-populated fields are fields that are added as dedicated fields to every record, unless it would conflict with a log or trace field that is marked as a custom field.
/// </summary>
public IReadOnlyDictionary<string, object> PrepopulatedFields
{
Expand Down
16 changes: 10 additions & 6 deletions src/OpenTelemetry.Exporter.Geneva/GenevaLogExporter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ namespace OpenTelemetry.Exporter.Geneva;
/// </summary>
public class GenevaLogExporter : GenevaBaseExporter<LogRecord>
{
internal readonly IDisposable Exporter;
internal bool IsUsingUnixDomainSocket;

private readonly ExportLogRecordFunc exportLogRecord;
private readonly IDisposable exporter;

private bool isDisposed;

Expand All @@ -46,7 +46,7 @@ public GenevaLogExporter(GenevaExporterOptions options)
var eventHeaderLogExporter = new EventHeaderLogExporter(options);
this.IsUsingUnixDomainSocket = false;
this.exportLogRecord = eventHeaderLogExporter.Export;
this.exporter = eventHeaderLogExporter;
this.Exporter = eventHeaderLogExporter;
return;
#else
throw new ArgumentException("Exporting data in user_events is only supported for .NET 8 or later on Linux.");
Expand Down Expand Up @@ -90,17 +90,21 @@ public GenevaLogExporter(GenevaExporterOptions options)

if (useMsgPackExporter)
{
var msgPackLogExporter = new MsgPackLogExporter(options);
var msgPackLogExporter = new MsgPackLogExporter(options, () =>
Copy link
Member

Choose a reason for hiding this comment

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

Should this have a null check? If ParentProvider hasn't been set when export is called (possible during SDK initialization edge cases), this will throw NRE. Consider:

() => this.ParentProvider?.GetResource() ?? Resource.Empty

Choose a reason for hiding this comment

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

I'll change it but I notice that if ParentProvider can really be null, then the existing metric code is also broken: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/src/OpenTelemetry.Exporter.Geneva/Metrics/GenevaMetricExporter.cs#L112

Copy link
Member

Choose a reason for hiding this comment

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

What does this mean? Is it a bug or not?

Copy link
Member

Choose a reason for hiding this comment

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

This is not a bug; the wording may have caused the confusion.
In the existing implementation, this.ParentProvider.GetResource() itself already returns Resource.Empty when no provider is set, so it is safe. We don’t need to add an explicit null check here or manually set Resource.Empty again.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @rajkumar-rangaraj! Then this sounds wrong and should be reverted back.

I'll change it

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this needs a fix. I will send a follow-up PR.

{
// this is not equivalent to passing a method reference, because the ParentProvider could change after the constructor.
return this.ParentProvider.GetResource();
});
this.IsUsingUnixDomainSocket = msgPackLogExporter.IsUsingUnixDomainSocket;
this.exportLogRecord = msgPackLogExporter.Export;
this.exporter = msgPackLogExporter;
this.Exporter = msgPackLogExporter;
}
else
{
var tldLogExporter = new TldLogExporter(options);
this.IsUsingUnixDomainSocket = false;
this.exportLogRecord = tldLogExporter.Export;
this.exporter = tldLogExporter;
this.Exporter = tldLogExporter;
}
}

Expand All @@ -124,7 +128,7 @@ protected override void Dispose(bool disposing)
{
try
{
this.exporter.Dispose();
this.Exporter.Dispose();
}
catch (Exception ex)
{
Expand Down
15 changes: 10 additions & 5 deletions src/OpenTelemetry.Exporter.Geneva/GenevaTraceExporter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ public class GenevaTraceExporter : GenevaBaseExporter<Activity>
{
internal readonly bool IsUsingUnixDomainSocket;

internal readonly IDisposable Exporter;

private readonly ExportActivityFunc exportActivity;
private readonly IDisposable exporter;

private bool isDisposed;

Expand Down Expand Up @@ -68,17 +69,21 @@ public GenevaTraceExporter(GenevaExporterOptions options)

if (useMsgPackExporter)
{
var msgPackTraceExporter = new MsgPackTraceExporter(options, this.ParentProvider.GetResource);
var msgPackTraceExporter = new MsgPackTraceExporter(options, () =>
{
// this is not equivalent to passing a method reference, because the ParentProvider could change after the constructor.
return this.ParentProvider.GetResource();
});
this.IsUsingUnixDomainSocket = msgPackTraceExporter.IsUsingUnixDomainSocket;
this.exportActivity = msgPackTraceExporter.Export;
this.exporter = msgPackTraceExporter;
this.Exporter = msgPackTraceExporter;
}
else
{
var tldTraceExporter = new TldTraceExporter(options);
this.IsUsingUnixDomainSocket = false;
this.exportActivity = tldTraceExporter.Export;
this.exporter = tldTraceExporter;
this.Exporter = tldTraceExporter;
}
}

Expand All @@ -102,7 +107,7 @@ protected override void Dispose(bool disposing)
{
try
{
this.exporter.Dispose();
this.Exporter.Dispose();
}
catch (Exception ex)
{
Expand Down
Loading