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

[Exporter.Geneva] LogExporter to serialize Exception Stack #672

Merged
merged 17 commits into from
Oct 19, 2022

Conversation

cijothomas
Copy link
Member

@cijothomas cijothomas commented Sep 30, 2022

Fixes #439

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #

@github-actions github-actions bot requested a review from CodeBlanch October 7, 2022 21:55
@cijothomas cijothomas marked this pull request as ready for review October 10, 2022 20:05
@cijothomas cijothomas requested a review from a team October 10, 2022 20:05
@cijothomas
Copy link
Member Author

Failed OpenTelemetry.Instrumentation.Wcf.Tests.TelemetryClientMessageInspectorTests.OutgoingRequestInstrumentationTest(instrument: True, filter: False, suppressDownstreamInstrumentation: True, includeVersion: False, enrich: False, enrichmentException: False, emptyOrNullAction: False) [1 ms]
Error Message:
System.ObjectDisposedException : Cannot access a disposed object.
Object name: 'System.Net.HttpListener'.
Stack Trace:
at System.Net.HttpListener.CheckDisposed()
at System.Net.HttpListener.Stop()
at OpenTelemetry.Instrumentation.Wcf.Tests.TelemetryClientMessageInspectorTests..ctor() in D:\a\opentelemetry-dotnet-contrib\opentelemetry-dotnet-contrib\test\OpenTelemetry.Instrumentation.Wcf.Tests\TelemetryClientMessageInspectorTests.cs:line 57
at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, Boolean wrapExceptions)

unrelated

@cijothomas
Copy link
Member Author

[xUnit.net 00:00:02.05] OpenTelemetry.Instrumentation.AspNet.Tests.HttpInMetricsListenerTests.HttpDurationMetricIsEmitted [FAIL]
[xUnit.net 00:00:02.06] OpenTelemetry.Instrumentation.AspNet.Tests.HttpInListenerTests.AspNetRequestsAreCollectedSuccessfully(expectedUrl: "https://localhost/", url: "***localhost/", routeType: 0, routeTemplate: null, setStatusToErrorInEnrich: False, filter: null, recordException: False) [FAIL]
Failed OpenTelemetry.Instrumentation.AspNet.Tests.HttpInMetricsListenerTests.HttpDurationMetricIsEmitted [356 ms]
Error Message:
The collection was expected to contain a single element, but it contained 2 elements.
Stack Trace:
at OpenTelemetry.Instrumentation.AspNet.Tests.HttpInMetricsListenerTests.HttpDurationMetricIsEmitted() in D:\a\opentelemetry-dotnet-contrib\opentelemetry-dotnet-contrib\test\OpenTelemetry.Instrumentation.AspNet.Tests\HttpInMetricsListenerTests.cs:line 69
Failed OpenTelemetry.Instrumentation.AspNet.Tests.HttpInListenerTests.AspNetRequestsAreCollectedSuccessfully(expectedUrl: "https://localhost/", url: "***localhost/", routeType: 0, routeTemplate: null, setStatusToErrorInEnrich: False, filter: null, recordException: False) [359 ms]
Error Message:
The collection was expected to contain a single element, but it contained 2 elements.
Stack Trace:
at OpenTelemetry.Instrumentation.AspNet.Tests.HttpInListenerTests.AspNetRequestsAreCollectedSuccessfully(String expectedUrl, String url, Int32 routeType, String routeTemplate, Boolean setStatusToErrorInEnrich, String filter, Boolean recordException) in D:\a\opentelemetry-dotnet-contrib\opentelemetry-dotnet-contrib\test\OpenTelemetry.Instrumentation.AspNet.Tests\HttpInListenerTests.cs:line 172

one more unrelated

@codecov
Copy link

codecov bot commented Oct 10, 2022

Codecov Report

Merging #672 (050fe2f) into main (d4045b8) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head 050fe2f differs from pull request most recent head 553a8a1. Consider uploading reports for the commit 553a8a1 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #672      +/-   ##
==========================================
+ Coverage   77.55%   77.60%   +0.05%     
==========================================
  Files         175      175              
  Lines        5310     5323      +13     
==========================================
+ Hits         4118     4131      +13     
  Misses       1192     1192              
Impacted Files Coverage Δ
...Telemetry.Exporter.Geneva/GenevaExporterOptions.cs 91.89% <100.00%> (+0.22%) ⬆️
...orter.Geneva/MsgPackExporter/MsgPackLogExporter.cs 88.55% <100.00%> (+0.61%) ⬆️

@utpilla utpilla added the comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva label Oct 10, 2022
@utpilla utpilla changed the title Exporter.Geneva - LogExporter to serialize Excepion Stack [Exporter.Geneva] LogExporter to serialize Exception Stack Oct 10, 2022
Assert.Equal(typeof(MyException).FullName, actualExceptionType);

var actualExceptionStack = exportedFields["env_ex_stack"];
Assert.EndsWith("...", (string)actualExceptionStack);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Add another Assert for exception stack length.

formatter: null);
},
(genevaOptions) =>
genevaOptions.ExceptionStackExportOption = ExceptionStackExportOptions.ExportAsString);
Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably update this test to a Theory to test both the exception stack export options.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 18, 2022
@utpilla utpilla removed the Stale label Oct 18, 2022
@github-actions github-actions bot requested a review from CodeBlanch October 19, 2022 19:09
Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot requested a review from CodeBlanch October 19, 2022 19:38
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 for preview version.

@cijothomas cijothomas merged commit 7272454 into open-telemetry:main Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export exception stack
6 participants