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

Improve the LogSqlDependency method signature #286

Closed
fgheysels opened this issue Feb 4, 2022 · 5 comments · Fixed by #376
Closed

Improve the LogSqlDependency method signature #286

fgheysels opened this issue Feb 4, 2022 · 5 comments · Fixed by #376
Assignees
Labels
dependencies All issues related to dependencies enhancement New feature or request
Milestone

Comments

@fgheysels
Copy link
Member

fgheysels commented Feb 4, 2022

The LogSqlDependency extension method on ILogger has a parameter which is called tableName.

This is a bit shortcoming as, when you want to log a dependency on SQL server, you execute (in most cases) a query that touches multiple tables.
This tableName parameter is used to build up the dependencyName:

image

It makes no sense to pass in a single tablename if you perform a SQL query that touches multiple tables, so it would be better if we:

  • rename the tableName parameter to something more generic: sqlCommand or sqlCommandInfo
  • Since that parameter can contain a lot more information, it should not be used to build up the dependencyName. Only the database-name should be in the dependency name.
  • The sqlCommand / sqlCommandInfo parameter can be logged as a contextual item / dimension
  • The sqlCommand/ sqlCommandInfo parameter should not be considered as mandatory, so we should not guard it.

See also this dicussion: #265

@fgheysels fgheysels added this to the v2.5.0 milestone Feb 4, 2022
@stijnmoreels stijnmoreels added dependencies All issues related to dependencies enhancement New feature or request labels Feb 4, 2022
@fgheysels
Copy link
Member Author

fgheysels commented Feb 4, 2022

I believe that, the operationName parameter becomes obsolete when we use a sqlCommandInfo parameter instead, as the sqlCommandInfo parameter might be a candidate for the dependencyData argument in the call to LogWarning

image

@stijnmoreels
Copy link
Member

stijnmoreels commented Apr 11, 2022

We can indeed move from the tableName towards something else, bc, like you said, it doesn't make sense when the query spans multiple tables. But, I have reservations on an optional contextual property being part of the signature. Couldn't this then be something that the consumer choses to add or not instead of making this explicit in the signature? SQL may need some other interesting contextual information, but I think it would be 'overload' to try to add these to the signature or provide an 'info'-something to encompass all this. If this would be used as dependency data solely, then yeah, that could be useful.

  • Remove tableName from SQL tracking
  • Remove operationName from SQL tracking
  • Add sqlCommand to SQL tracking

Something like this? (yes, of course with deprecation on the current ones).

@fgheysels
Copy link
Member Author

Don't know why this one is closed, as the tableName and operationName are still mandatory in this method.

@fgheysels fgheysels reopened this Jun 8, 2022
@fgheysels
Copy link
Member Author

Ah, it is just not released yet ?

@stijnmoreels
Copy link
Member

Ah, it is just not released yet ?

That's right. The other methods without the dependency ID aren't updated, though, so maybe we should also create alternatives for those ones?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies All issues related to dependencies enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants