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

Geneva Exporter - Throw on TableNameMappings null value #322

Merged
merged 13 commits into from
May 7, 2022
3 changes: 3 additions & 0 deletions src/OpenTelemetry.Exporter.Geneva/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

* Throw exception when `TableNameMappings` contains a `null` value.
[322](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/322)

* Update OTel SDK version to `1.2.0`.
[319](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/319)

Expand Down
3 changes: 3 additions & 0 deletions src/OpenTelemetry.Exporter.Geneva/GenevaLogExporter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
using System.Text;
using System.Threading;
using Microsoft.Extensions.Logging;
using OpenTelemetry.Internal;
using OpenTelemetry.Logs;

namespace OpenTelemetry.Exporter.Geneva;
Expand Down Expand Up @@ -64,6 +65,8 @@ public GenevaLogExporter(GenevaExporterOptions options)
var tempTableMappings = new Dictionary<string, string>(options.TableNameMappings.Count, StringComparer.Ordinal);
foreach (var kv in options.TableNameMappings)
{
Guard.ThrowIfNull(kv.Value);
Copy link
Contributor

@TimothyMothra TimothyMothra Apr 21, 2022

Choose a reason for hiding this comment

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

I would recommend including the Key name in the exception message here to help users identify which value is null.

Copy link
Contributor

@utpilla utpilla Apr 21, 2022

Choose a reason for hiding this comment

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

Would it be better to do this validation in the GenevaExporterOptions class, like how it's done for PrepopulatedFields?

https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/src/OpenTelemetry.Exporter.Geneva/GenevaExporterOptions.cs#L36-L73


if (Encoding.UTF8.GetByteCount(kv.Value) != kv.Value.Length)
{
throw new ArgumentException("The value: \"{tableName}\" provided for TableNameMappings option contains non-ASCII characters", kv.Value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@
<ItemGroup>
<PackageReference Include="OpenTelemetry" Version="1.2.0" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,14 @@ public void SpecialChractersInTableNameMappings()
{
TableNameMappings = new Dictionary<string, string> { ["*"] = "\u0418" },
});
});

Assert.Throws<ArgumentNullException>(() =>
{
using var exporter = new GenevaLogExporter(new GenevaExporterOptions
{
TableNameMappings = new Dictionary<string, string> { ["TestCategory"] = null },
});
});
}

Expand Down