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

ref: Handle non json error response messages. #690

Merged
merged 4 commits into from
Dec 28, 2020

Conversation

lucas-zimerman
Copy link
Collaborator

@lucas-zimerman lucas-zimerman commented Dec 26, 2020

Closes #689.
Closes #688

@lucas-zimerman lucas-zimerman added the Feature New feature or request label Dec 26, 2020
@codecov-io
Copy link

codecov-io commented Dec 26, 2020

Codecov Report

Merging #690 (eab3e84) into main (0847ae3) will increase coverage by 1.94%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #690      +/-   ##
==========================================
+ Coverage   76.01%   77.96%   +1.94%     
==========================================
  Files         163      163              
  Lines        5250     5260      +10     
  Branches     1350     1354       +4     
==========================================
+ Hits         3991     4101     +110     
+ Misses        879      769     -110     
- Partials      380      390      +10     
Impacted Files Coverage Δ
src/Sentry/Internal/Http/HttpTransport.cs 81.39% <84.61%> (-0.19%) ⬇️
src/Sentry/Internal/AppDomainAdapter.cs 66.66% <0.00%> (ø)
src/Sentry/PlatformAbstractions/Runtime.cs 43.90% <0.00%> (+2.43%) ⬆️
src/Sentry/PlatformAbstractions/RuntimeInfo.cs 58.62% <0.00%> (+5.17%) ⬆️
...grations/TaskUnobservedTaskExceptionIntegration.cs 84.61% <0.00%> (+7.69%) ⬆️
...ntry/PlatformAbstractions/FrameworkInstallation.cs 50.00% <0.00%> (+25.00%) ⬆️
...ntry/Integrations/NetFxInstallationsIntegration.cs 100.00% <0.00%> (+66.66%) ⬆️
...rmAbstractions/NetFxInstallationsEventProcessor.cs 72.72% <0.00%> (+68.18%) ⬆️
...Sentry/PlatformAbstractions/FrameworkInfo.NetFx.cs 71.23% <0.00%> (+71.23%) ⬆️
src/Sentry/PlatformAbstractions/FrameworkInfo.cs 100.00% <0.00%> (+100.00%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0847ae3...eab3e84. Read the comment docs.

@@ -68,17 +68,36 @@ internal static class DiagnosticLoggerExtensions
public static void LogError<TArg>(
this IDiagnosticLogger logger,
string message,
Exception exception,
Exception? exception,
Copy link
Member

Choose a reason for hiding this comment

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

Not supposed to be nullable.
We should have an overload that doesn't take it then


_options.DiagnosticLogger?.LogError(
"Sentry rejected the envelope {0}. Status code: {1}. Error detail: {2}. Error causes: {3}.",
null,
Copy link
Member

Choose a reason for hiding this comment

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

Lets have an overload that doesn't take null

@lucas-zimerman
Copy link
Collaborator Author

lucas-zimerman commented Dec 28, 2020

LogError Extensions are going to be implemented on another PR. For now, I'm using the old Log pattern.

@bruno-garcia bruno-garcia merged commit 8f752fb into main Dec 28, 2020
@bruno-garcia bruno-garcia deleted the ref-avoid-parsing-text-as-json-http branch December 28, 2020 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request
Projects
None yet
3 participants