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.

Sign up for GitHub

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

Jump to bottom

[ExporterGeneva]Remove ConvertToJson option #514

Merged
merged 27 commits into from
Jul 28, 2022
Merged

[ExporterGeneva]Remove ConvertToJson option #514

merged 27 commits into from
Jul 28, 2022

Conversation

xyq175com
Copy link
Contributor

@xyq175com xyq175com commented Jul 19, 2022 •
edited
Loading

[ExporterGeneva]Remove ConvertToJson option

Fix issue with #395

Changes

Remove ConvertToJson option

@xyq175com xyq175com requested a review from a team July 19, 2022 01:34
@codecov
Copy link

codecov bot commented Jul 19, 2022 •
edited
Loading

Codecov Report

Merging #514 (9b96d60) into main (8ce7845) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #514   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files        167     167           
  Lines       5099    5097    -2     
=====================================
+ Misses      5099    5097    -2
Impacted Files Coverage Δ
...Telemetry.Exporter.Geneva/GenevaExporterOptions.cs 0.00% <0.00%> (ø)
...OpenTelemetry.Exporter.Geneva/GenevaLogExporter.cs 0.00% <ø> (ø)
...enTelemetry.Exporter.Geneva/GenevaTraceExporter.cs 0.00% <ø> (ø)

@utpilla utpilla added the comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva label Jul 20, 2022
@xyq175com xyq175com requested a review from utpilla July 21, 2022 03:39
reyang
reyang reviewed Jul 26, 2022
View reviewed changes
src/OpenTelemetry.Exporter.Geneva/GenevaExporterOptions.cs
case string vs:
break;
default:
throw new ArgumentException($"Type: {entry.Value.GetType()} is not supported. Only bool, byte, sbyte, short, ushort, int, uint, long, ulong, float, double, and string are the supported types for PrepopulatedFields values.");
Copy link
Member

Choose a reason for hiding this comment

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

would this throw NullReferenceException if value is null?

Copy link
Member

Choose a reason for hiding this comment

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

Is this addressed?

Copy link
Member

Choose a reason for hiding this comment

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

#537

reyang
reyang reviewed Jul 26, 2022
View reviewed changes
src/OpenTelemetry.Exporter.Geneva/GenevaExporterOptions.cs
case string vs:
break;
default:
throw new ArgumentException($"Type: {entry.Value.GetType()} is not supported. Only bool, byte, sbyte, short, ushort, int, uint, long, ulong, float, double, and string are the supported types for PrepopulatedFields values.");
Copy link
Member

Choose a reason for hiding this comment

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

"for PrepopulatedFields values" can be removed since this is self-explanatory.

Copy link
Member

Choose a reason for hiding this comment

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

Is this addressed?

Copy link
Member

Choose a reason for hiding this comment

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

#537

reyang
reyang approved these changes Jul 26, 2022
View reviewed changes
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM with minor suggestions.

@utpilla
Copy link
Contributor

utpilla commented Jul 27, 2022

Add a unit test for this.

xyq175com added 2 commits July 27, 2022 08:19
@xyq175com
fix_issue_395
6f24239 
Remove Internal function ConvertToJson as this is no longer needed
@xyq175com
Merge branch 'open-telemetry:main' into fix_issue_395
b7dad9e
@xyq175com
Copy link
Contributor Author

xyq175com commented Jul 27, 2022 •
edited
Loading

Add a unit test for this.

There is no existing Unit test for this file. I have to see that add unit test is hard for me.;), Can you help approve or give some suggestion for this, Thank you so much!

@utpilla
Copy link
Contributor

utpilla commented Jul 27, 2022

Add a unit test for this.

There is no existing Unit test for this file. I have to see that add unit test is hard for me.;), Can you help approve or give some suggestion for this, Thank you so much!

You're right! Currently, there is no dedicated test class for this file. However, you could still add a unit test in GenevaTraceExporter for validating this feature. You can refer to this existing unit test: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/test/OpenTelemetry.Exporter.Geneva.Tests/GenevaTraceExporterTests.cs#L39-L56 and pass in some unsupported type in PrepopulatedFields and ensure that it throws an exception. Add a case for happy path as well.

@xyq175com xyq175com requested a review from utpilla July 27, 2022 13:20
@xyq175com
Update GenevaTraceExporterTests.cs
9b96d60 
Remove unnecessary test
@xyq175com xyq175com requested a review from utpilla July 27, 2022 23:40
@utpilla utpilla merged commit a55cbad into open-telemetry:main Jul 28, 2022
@reyang reyang mentioned this pull request Jul 28, 2022
Merged
@xyq175com xyq175com deleted the fix_issue_395 branch August 3, 2022 08:18
samimusallam pushed a commit to samimusallam/opentelemetry-dotnet-contrib that referenced this pull request Aug 9, 2022
@xyq175com @samimusallam
[ExporterGeneva]Remove ConvertToJson option (open-telemetry#514)
51e1dec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reyang reyang reyang approved these changes

@utpilla utpilla utpilla approved these changes

@cijothomas cijothomas cijothomas approved these changes

@CodeBlanch CodeBlanch Awaiting requested review from CodeBlanch

@mic-max mic-max Awaiting requested review from mic-max

@Yun-Ting Yun-Ting Awaiting requested review from Yun-Ting

Assignees

@cijothomas cijothomas

@mic-max mic-max

@Yun-Ting Yun-Ting

@reyang reyang

@CodeBlanch CodeBlanch

@utpilla utpilla

Labels
comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@xyq175com @utpilla @cijothomas @reyang @mic-max @Yun-Ting @CodeBlanch