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

Fix missing logs issue #34423

Merged
merged 2 commits into from
Feb 21, 2023
Merged

Fix missing logs issue #34423

merged 2 commits into from
Feb 21, 2023

Conversation

vishweshbankwar
Copy link
Contributor

@vishweshbankwar vishweshbankwar commented Feb 21, 2023

#34259 reported that logs are not exported if any one of the logs in the received batch hit an exception when we convert the log to Azure Monitor trace. This PR addresses this by handling the exception individually for each log rather than failing the entire batch.

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@vishweshbankwar vishweshbankwar merged commit b5a0180 into main Feb 21, 2023
@vishweshbankwar vishweshbankwar deleted the vibankwa/fix-34259 branch February 21, 2023 19:36
@reyang
Copy link
Member

reyang commented Feb 21, 2023

@cijothomas FYI you've mentioned that #34259 might depend on a fix in the OpenTelemetry .NET SDK, @vishweshbankwar do we know if this PR is going to fix #34259 or we need other changes?

@vishweshbankwar
Copy link
Contributor Author

@cijothomas FYI you've mentioned that #34259 might depend on a fix in the OpenTelemetry .NET SDK, @vishweshbankwar do we know if this PR is going to fix #34259 or we need other changes?

We need other changes. There are two parts that need a fix

  1. Missing logs (This is a side effect of error shown in the issue) --> Fixed by this PR
  2. Error shown in the issue --> This needs further investigation.
Object name: 'Collection'.
   at Microsoft.AspNetCore.Http.Features.FeatureReferences`1.ThrowContextDisposed()
   at Microsoft.AspNetCore.Http.Features.FeatureReferences`1.ContextDisposed()
   at Microsoft.AspNetCore.Http.Features.FeatureReferences`1.Fetch[TFeature](TFeature& cached, Func`2 factory)
   at Microsoft.AspNetCore.Http.DefaultHttpRequest.get_Protocol()
   at Microsoft.AspNetCore.Hosting.HostingRequestStartingLog.get_Item(Int32 index)
   at Microsoft.AspNetCore.Hosting.HostingRequestStartingLog.GetEnumerator()+MoveNext()
   at Azure.Monitor.OpenTelemetry.Exporter.Internals.LogsHelper.ExtractProperties(String& message, IDictionary`2 properties, IReadOnlyCollection`1 stateDictionary)
   at Azure.Monitor.OpenTelemetry.Exporter.Internals.LogsHelper.GetMessageAndSetProperties(LogRecord logRecord, IDictionary`2 properties)
   at Azure.Monitor.OpenTelemetry.Exporter.Models.MessageData..ctor(Int32 version, LogRecord logRecord)
   at Azure.Monitor.OpenTelemetry.Exporter.Internals.LogsHelper.OtelToAzureMonitorLogs(Batch`1 batchLogRecord, AzureMonitorResource resource, String instrumentationKey)
   at Azure.Monitor.OpenTelemetry.Exporter.AzureMonitorLogExporter.Export(Batch`1& batch)]

@reyang
Copy link
Member

reyang commented Feb 21, 2023

We need other changes. There are two parts that need a fix

Got it, would you reopen the issue + update the PR description here to reflect the intention? Thanks!

@vishweshbankwar
Copy link
Contributor Author

vishweshbankwar commented Feb 21, 2023

We need other changes. There are two parts that need a fix

Got it, would you reopen the issue + update the PR description here to reflect the intention? Thanks!

@reyang I have updated the description here and sent another PR to handle part 2 i.e. the exception.

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.

5 participants