Skip to content
This repository has been archived by the owner on Jul 5, 2020. It is now read-only.

Fix dependency tracking if non-zero length response was cached by PropertyFetcher before #919

Merged
merged 3 commits into from
May 29, 2018

Conversation

lmolkova
Copy link
Member

PropertyFetcher must be created for each property in each event, the same property fetcher cannot be used for 2 different events.
In this case, if request fetcher was first used for 'System.Net.Http.Desktop.HttpRequestOut.Stop' event, it won't be able to fetch Request from 'System.Net.Http.Desktop.HttpRequestOut.Ex.Stop' event and vice versa.

  • I ran Unit Tests locally.

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

  • Changes in public surface reviewed

  • Design discussion issue #

  • CHANGELOG.md updated with one line description of the fix, and a link to the original issue.

  • The PR will trigger build, unit tests, and functional tests automatically. If your PR was submitted from fork - mention one of committers to initiate the build for you.
    If you want to to re-run the build/tests, the easiest way is to simply Close and Re-Open this same PR. (Just click 'close pull request' followed by 'open pull request' buttons at the bottom of the PR)

  • Please follow [these] (https://github.com/Microsoft/ApplicationInsights-dotnet-server/blob/develop/CONTRIBUTING.md) instructions to build and test locally.

@lmolkova lmolkova requested review from TimothyMothra and SergeyKanzhelev and removed request for TimothyMothra May 25, 2018 18:26
@chriswelles
Copy link

Is there any location for nightly build nugets or something? I'd really like to try this out. Also, do you have an expected timeline for moving this into master? App Insights completely skips logging a significant number of our HTTP calls because of this issue. It makes identifying issues and performance bottlenecks rather challenging. Thanks!

@TimothyMothra
Copy link
Member

@chriswelles as of today we don't publish nightly builds.

THIS change specifically will be included as part of 2.7-Beta1. I'm prepping that release today and you should see it published to NuGet early next week. :)

@zell12
Copy link

zell12 commented Jun 9, 2018

Could this be a related issue? See attached screenshot.
image

@chriswelles
Copy link

chriswelles commented Jun 11, 2018

I've just tried 2.7-Beta1. It hasn't fixed the problem. I'm still seeing the exceptions occur, and key HTTP dependency calls are not being tracked.

CORRECTION:
It does work. I didn't explicitly update the DependencyCollector nuget before. It didn't show up as an available pre-release update since I wasn't referencing it directly. Of course, I figured that immediately after I said that it didn't work :-) Ah well.

@chriswelles
Copy link

FYI - When I updated the Microsoft.ApplicationInsights.AspNetCore to the 2.4.0-beta1, I stopped seeing HTTP dependencies entirely. I haven't spent any time trying to diagnose that. The other beta components seem to work OK by themselves.

@lmolkova
Copy link
Member Author

lmolkova commented Jun 15, 2018

@chriswelles,

I've tried to repro it with 2.4.0 with no luck - I see all my http dependencies are collected.

If you have time to investigate, could you please create a new issue and let us know .NET version, how the app is hosted (AppSevice, Cloud Service, etc).
If you can repro issue locally, could you please collect some more logs?

  1. Download and run PerfView tool: https://github.com/Microsoft/perfview/blob/master/documentation/Downloading.md
  2. Choose Collect –> Collect (you might need to run it under admin more or accept file system permissions)
  3. In the new dialog expand “Advanced Options”, uncheck all boxes
  4. Add "Additional Provides": *Microsoft-ApplicationInsights-Extensibility-AppMapCorrelation-Web,*Microsoft-ApplicationInsights-Extensibility-DependencyCollector,*Microsoft-ApplicationInsights-WindowsServer-TelemetryChannel,*Microsoft-AspNet-Telemetry-Correlation,*Microsoft-ApplicationInsights-Data
  5. Start service
  6. When you are ready, click 'Start collection'
  7. Repro the issue
  8. Wait some time (e.g. 1 minute)
  9. Stop collection

Please send me generated etl file.

@chriswelles
Copy link

I'm sorry I haven't gotten to this yet. I've been completely swamped. I'm hoping to get a chance to do it tomorrow.

@chriswelles
Copy link

Well. I don't know what the issue was before, but I tried the 2.7-Beta1 ApplicationInsights.AspNetCore with the 2.7-Beta2 ApplicationInsights and DependencyCollector and it all seems to work. Maybe something new with the Beta2 or maybe I somehow mis-tested. Either way, it seems like things are looking good now. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants