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
Merged

Geneva Exporter - Throw on TableNameMappings null value #322

merged 13 commits into from
May 7, 2022

Conversation

mic-max
Copy link
Contributor

@mic-max mic-max commented Apr 20, 2022

Throw exception when null value is used in TableNameMappings for Geneva Exporter Options class.
Includes unit test.

@cijothomas
Copy link
Member

@mic-max please address conflicts.

@@ -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

@mic-max
Copy link
Contributor Author

mic-max commented Apr 27, 2022

What should be done when a null key exists in the TableNameMappings, if anything?

@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #322 (57c541d) into main (fc6a092) will increase coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #322      +/-   ##
==========================================
+ Coverage   24.09%   24.25%   +0.16%     
==========================================
  Files         123      123              
  Lines        3690     3698       +8     
==========================================
+ Hits          889      897       +8     
  Misses       2801     2801              
Impacted Files Coverage Δ
...Telemetry.Exporter.Geneva/GenevaExporterOptions.cs 90.62% <100.00%> (+3.12%) ⬆️

@cijothomas cijothomas merged commit bb01f81 into open-telemetry:main May 7, 2022
@mic-max mic-max deleted the throw-null-log-prepop branch May 11, 2022 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants