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

Added the ability to track dependency calls #46

Merged
merged 9 commits into from
Aug 17, 2024

Conversation

m-gug
Copy link
Contributor

@m-gug m-gug commented Nov 12, 2023

Remote dependency calls can now be tracked with trackDependency().

Background:
With trackRequest I had problems with distributed tracing across multiple application insights instances. AI could not connect the individual entries across the instances.
According to the Application Insights documentation, dependencies should be used to track calls to an HTTP endpoint. Requests are more intended to track incoming requests in the backend.
(https://learn.microsoft.com/en-us/azure/azure-monitor/app/data-model-complete#dependency)

With this change, distributed tracing also worked without any problems.

@m-gug
Copy link
Contributor Author

m-gug commented Jan 9, 2024

@kentcb any chance to get an approval for this PR or do you need anything?

@m-gug
Copy link
Contributor Author

m-gug commented May 17, 2024

@kentcb any chance, you could take a look at this PR? Should be a non breaking extension.

@m-gug
Copy link
Contributor Author

m-gug commented Aug 10, 2024

@johnnyggalt awesome!
I had some troubles with Application Insights not building the Correlation Context correctly and beeing unable to connect events accross multiple systems of Application Insights instances.
got the Sdk-Context from looking at the Application Insights JS SDK and the source code so i gave it a try. But to be honest it was more a shot in the dark. Here ist the reference to the Javascript SDK: https://github.com/microsoft/ApplicationInsights-JS/blob/54ea6d7eb46a7f2ba697e25dd082365e655a7bb4/shared/AppInsightsCommon/src/RequestResponseHeaders.ts#L39

It could easily be that this would not be necessary. The v2 JS SDK sent the header by default, I just saw that in the v3 SDK it is no longer included by default.
I have been using my fork with these changes in a production app since November 2023 and everything works perfectly.

@kentcb
Copy link
Owner

kentcb commented Aug 17, 2024

@m-gug Got it, thanks. I've removed it for now, since as best I can tell it is an internal thing, and worst case scenario is it might cause issues for folks with strict middleware. Anyway, if it turns out to be necessary, it can always be addressed separately to this PR.

@kentcb kentcb merged commit c3015de into kentcb:master Aug 17, 2024
1 check passed
@kentcb
Copy link
Owner

kentcb commented Aug 17, 2024

@all-contributors Please add @m-gug for code

Copy link
Contributor

@kentcb

I've put up a pull request to add @m-gug! 🎉

@m-gug
Copy link
Contributor Author

m-gug commented Aug 19, 2024

@m-gug Got it, thanks. I've removed it for now, since as best I can tell it is an internal thing, and worst case scenario is it might cause issues for folks with strict middleware. Anyway, if it turns out to be necessary, it can always be addressed separately to this PR.

Thank you, and no problem - i will check if the telemetry correlation is still working as intended on my side.

@m-gug
Copy link
Contributor Author

m-gug commented Aug 19, 2024

@kentcb do you already have a plan when you will release the new version to pub.dev?

@kentcb
Copy link
Owner

kentcb commented Aug 26, 2024

@m-gug I intend following up on the remaining open issue before releasing. Can't promise any dates though.

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.

2 participants