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

Make TrackDependency harder to use incorrectly #684

Closed
OsvaldoRosado opened this issue Dec 20, 2017 · 3 comments
Closed

Make TrackDependency harder to use incorrectly #684

OsvaldoRosado opened this issue Dec 20, 2017 · 3 comments
Assignees
Milestone

Comments

@OsvaldoRosado
Copy link
Member

OsvaldoRosado commented Dec 20, 2017

The definition of TrackDependency has a few small issues that make it surprisingly hard to use.

Starting with the first signature:
https://github.com/Microsoft/ApplicationInsights-dotnet/blob/a0c340ab7c415260cd2d1961c092dafd1858909e/src/Microsoft.ApplicationInsights/TelemetryClient.cs#L309

This signature, which has the fewest number of parameters (which I think makes it first to appear in IntelliSense), is missing the critical dependency type name parameter. When this data is missing (eg. set to "OTHER"), it degrades our Application Map and Details experiences in the portal. While not required, I find it unlikely for users to realize this implication, or perhaps even notice it's a field they can provide if they don't view the alternative function signatures.

Even worse, when combined with the example we give in our Azure docs (https://docs.microsoft.com/en-us/azure/application-insights/app-insights-api-custom-events-metrics#trackdependency ) there is the strong insinuation that the first parameter of this short definition actually is for the dependency type! I've seen exactly this done in customer code.

This signature also includes a commandName parameter, which doesn't appear to have an analog in the full signature after it. I presume this should be data? As it is now, it looks like if you move to the full signature, you have to give up this field, even though it's actually just inconsistently named.

On to the second (full) signature:
https://github.com/Microsoft/ApplicationInsights-dotnet/blob/a0c340ab7c415260cd2d1961c092dafd1858909e/src/Microsoft.ApplicationInsights/TelemetryClient.cs#L330

This signature includes all of the fields for making a complete dependency telemetry item, but doesn't really differentiate between its parameters. In particular, note the definition for dependencyName as "External dependency name" and data as "Dependency call command name". These are incredibly confusing! I think we do a far better job defining these in the BOND schema (https://github.com/Microsoft/ApplicationInsights-Home/blob/master/EndpointSpecs/Schemas/Docs/RemoteDependencyData.md)

I think we should:

  1. Mark the current short signature of TrackDependency as Obsolete, and add a new short signature that has the same fields, as well as the dependencyTypeName field. I'm not sure what real scenarios exist where you have a dependency of unknown type, but for this edge case users can still provide null/string.Empty to the parameter.
  2. Unify our parameter names, is it or is it not data/commandName?
  3. Replace the current parameter definitions with ones derived from our schema definitions. Our BOND schema provides great definitions with even examples of what it is looking for in each field. We should take advantage of this!

I can submit a PR for this if others are on board.

@OsvaldoRosado OsvaldoRosado changed the title Make trackDependency harder to use incorrectly Make TrackDependency harder to use incorrectly Dec 20, 2017
@TimothyMothra TimothyMothra added this to the 2.6-Beta1 milestone Dec 20, 2017
@SergeyKanzhelev
Copy link
Contributor

Ship it

@zakimaksyutov
Copy link
Member

+1!

@TimothyMothra
Copy link
Member

@OsvaldoRosado can we close this issue now? :)

TimothyMothra pushed a commit that referenced this issue Oct 25, 2019
Add retry logic if FakeListener cannot be started. There are random t…
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

No branches or pull requests

4 participants