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

SQL/Stored Procedure - Parameters and Values #193

Closed
leikelman opened this issue Oct 29, 2016 · 7 comments
Closed

SQL/Stored Procedure - Parameters and Values #193

leikelman opened this issue Oct 29, 2016 · 7 comments
Milestone

Comments

@leikelman
Copy link

We are looking at some of the statistics from AI and noticed that in the SQL information coming from the dependency monitoring the stored procedures are not showing the parameters that were used to call the stored procedure. Is there some way to have AI display the full SQL call. Knowing the stored procedure is helpful, but it is only half of the required information if we don't know which parameters were passed into the procedure.

Thank you.

@adminnz
Copy link

adminnz commented Feb 15, 2017

If Microsoft are not willing to implement such a feature. Then it would be good if the ProfilerSqlCommandProcessing or ProfilerSqlProcessingBase were not internal & sealed. So we could inherit and override with specific implementations.
Then the only thing left would be to provide the ability to configure the DependencyTrackingTelemetryModule at runtime (maybe read a setup module from config, and if defined then call the setup module with the current in DependencyTrackingTelemetryModule instance.

@Dmitry-Matveev Dmitry-Matveev modified the milestones: 107, 2.5, 2.5-Beta1 Apr 24, 2017
@AlexBulankou AlexBulankou modified the milestone: 2.5-Beta1 Jul 3, 2017
@Dmitry-Matveev Dmitry-Matveev added this to the Future milestone Oct 12, 2017
@Dmitry-Matveev
Copy link
Member

Parameters of stored procedure calls may contain Personally Identifiable Information (PII) and it may be hard to justify such a collection by default. With this thinking, Event Source based dependency modules do not collect Command Text by default unlike profiler modules do. Users of the profiler instrumentation had asks about masking the parameters in the command text to avoid collection of PII.

However, this may become and opt-in feature for the users who understand the risks or do not have PII in the parameters. Keeping it as enhancement for the Future milestone.

@SergeyKanzhelev to answer on suggestions around the architecture and public/internal discussion.

@SergeyKanzhelev
Copy link
Contributor

I think we need to implement callback like mentioned here: https://github.com/Microsoft/ApplicationInsights-dotnet-server/issues/343 So that telemetry initializers will get the original object. So custom telemetry extraction can be implemented. We also discussed it here: microsoft/ApplicationInsights-dotnet-logging#111

@adminnz would this callback be better than re-implementing internal classes?

As @Dmitry-Matveev said we unlikely will enable parameters collection without some form of consent (config setting or such)

@lmolkova
Copy link
Member

This was fixed in #900.
You can access SqlCommand operation detail in TelemetryInitializer. Note, they will be gone before processors chain starts.

    public class SqlParametersTelemetryInitializer : ITelemetryInitializer
    {
        public void Initialize(ITelemetry telemetry)
        {
            if (telemetry is DependencyTelemetry dependencyTelemetry && dependencyTelemetry .Type == "SQL")
            {
                if (dependencyTelemetry.TryGetOperationDetail("SqlCommand", out var command)
                    && command is SqlCommand sqlCommand)
                {
                    ...
                }
            }
        }
    }

@SashaPshenychniy
Copy link

@lmolkova When I initialize Target and Name of dependency in order to achieve grouping by SQL command (I take Target from ConnectionString, Name = SQL statement hash), I get some ridiculous ApplicationMap. It seems some of the telemetry items are overwritten by telemetryProcessors you mention. Is there any way to avoid that?

@cijothomas
Copy link
Contributor

@SashaPshenychniy can you elaborate what is being overwritten? The operationdetails stored in dependencyTelemetry will be gone after TelemetryProcessors starts, but if you access it in a TelemetryInitializer and copy it to other fields like Name or Target, then there shouldn't be anything in sdk which overwrites these.

@SashaPshenychniy
Copy link

@cijothomas , Thank you for your help. All in all, it appeared to be a set of issues resulting in very strange application map:
image

  • Root cause, which is still a source of disappointment... When I set telemetry.Name to something like "SELECT TableA|TableB|TableC" (using vertical dash as separator), it would often "magically" corrupt Target and Command properties, resulting in many instances of the same SQL DB on application map. If I use other separator for tables used, e.g. "^", everything is as expected. So my problem was "|" symbol in Name.

  • I attempted to fix Target by initializing it from sqlCommand.Connection.ConnectionString (to build Target), which sometimes appeared to be empty due to closed SQL connection. At the end I discovered that it wasn't problem with Target originally, so I left it untouched and it is being initialized automatically exactly as I wanted.

  • Last was non-deterministic hashing algorithm for .NET Core (I added SQL Command hash into each Name to avoid different queries on same tables having same name). https://andrewlock.net/why-is-string-gethashcode-different-each-time-i-run-my-program-in-net-core/

All in all I'm happy I managed to make SQL dependency group SQL queries by SQL command and I'm just curious why Microsoft didn't implement it out of the box - it didn't appear that hard at the end.

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

No branches or pull requests

8 participants