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

Dependency data in DependencyMeasurement is not always used #279

Closed
stijnmoreels opened this issue Jan 27, 2022 · 6 comments
Closed

Dependency data in DependencyMeasurement is not always used #279

stijnmoreels opened this issue Jan 27, 2022 · 6 comments
Labels
application-insights All issues related to Azure Application Insights dependencies All issues related to dependencies management All issues related to management of the project
Milestone

Comments

@stijnmoreels
Copy link
Member

Describe the bug
When tracking an dependency, you can use the DependencyMeasurement tracking model. This contains a way of specifying a dependency data value, but this value doesn't always corresponds with the dependency data of Application Insights.

To Reproduce
Assign dependency data via the DependencyMeasurement tracking model (DependencyMeasurement.Start(dependencyData: "Process") and look for the dependency data upon logging.

Expected behavior
Passed-allong dependency data matches.

Additional context
Add any other context about the problem here.

  • v2.3.0, current master.
@stijnmoreels stijnmoreels added management All issues related to management of the project application-insights All issues related to Azure Application Insights dependencies All issues related to dependencies labels Jan 27, 2022
@stijnmoreels stijnmoreels added this to the v2.4.0 milestone Jan 27, 2022
@stijnmoreels
Copy link
Member Author

stijnmoreels commented Jan 27, 2022

In almost all cases, there is already something passed-along to the dependency data (the table name, in case of Azure Table Storage, for example), which raises the question if we should have a dependency data in the measurement after all...

public static void LogEventHubsDependency(
            this ILogger logger, 
            string namespaceName, 
            string eventHubName, 
            bool isSuccessful, 
            DateTimeOffset startTime, 
            TimeSpan duration, 
            Dictionary<string, object> context = null)
        {
            logger.LogWarning(DependencyFormat, new DependencyLogEntry(
                dependencyType: "Azure Event Hubs",
                dependencyName: eventHubName,
                dependencyData: namespaceName, // <-- already assigned.
                targetName: eventHubName,
                duration: duration,
                startTime: startTime,
                resultCode: null,
                isSuccessful: isSuccessful,
                context: context));
        }

@fgheysels
Copy link
Member

Actually, I do not see why you want to add additional dependency - data to the measurement.
The measurement should just measure the duration of the dependency call, and that measurement should be used when writing the dependency tracking to the logger.
At this moment, I do not see a reason why we would have this dependency data in the first place.

On the other hand, is it necessary to remove this right now ?
Also, if we remove this, is there still a reason to differentiate between RequestMeasurement & DependencyMeasurement ? We could create one type Measurement or DurationMeasurement for this.

@stijnmoreels
Copy link
Member Author

stijnmoreels commented Jan 27, 2022

Reusability comes to mind. If you want to track several dependencies with the same data.
But otherwise, yeah, something like TelemetryMeasurement can be made instead. 😉

I would want to do this change before the release bc otherwise we end up with RequestMeasurement that would directly be made obsolete.

@fgheysels
Copy link
Member

True.

So, how do we agree on this ?
We rename the RequestMeasurement to DurationMeasurement, and use DurationMeasurement also in measuring dependencies , so the DependencyMeasurement class, and methods that use DependencyMeasurement become obsolete ?

@stijnmoreels
Copy link
Member Author

Yes, I think that would be good. It would take a bit of work to mark all those signatures obsolete and create new ones. But yeah, that would be good to do this. 👍

@stijnmoreels
Copy link
Member Author

Ok, let's close this in favor of #283 and #284 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application-insights All issues related to Azure Application Insights dependencies All issues related to dependencies management All issues related to management of the project
Projects
None yet
Development

No branches or pull requests

2 participants