Skip to content

SqlClient public API renaming proposal for db.statement properties - #1645

Merged
cijothomas merged 10 commits into
open-telemetry:masterfrom
mbakalov:sqlclient-statementapi
Jan 20, 2021
Merged

SqlClient public API renaming proposal for db.statement properties#1645
cijothomas merged 10 commits into
open-telemetry:masterfrom
mbakalov:sqlclient-statementapi

Conversation

@mbakalov

@mbakalov mbakalov commented Dec 6, 2020

Copy link
Copy Markdown
Contributor

Discussion point raised in: #1599, regarding properties that control setting of the db.statement semantic convention.

Current SqlClientInstrumentationOptions public API looks like this:

  • netcore
    • SetStoredProcedureCommandName
    • SetTextCommandContent
  • netfx
    • SetStatementText (controls both sp and sqltext as there is no way to tell the difference in netfx)

The suggestion is to make this more uniform and rename to:

  • netcore

    • SetDbStatementForStoredProcedure
    • SetDbStatementForText
  • netfx

    • SetDbStatement
  • Changes in public API reviewed

@codecov

codecov Bot commented Dec 6, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1645 (4a7f787) into master (c7e5f07) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1645   +/-   ##
=======================================
  Coverage   82.14%   82.14%           
=======================================
  Files         250      250           
  Lines        6748     6748           
=======================================
  Hits         5543     5543           
  Misses       1205     1205           
Impacted Files Coverage Δ
...ient/Implementation/SqlClientDiagnosticListener.cs 77.94% <100.00%> (ø)
...ation.SqlClient/SqlClientInstrumentationOptions.cs 98.64% <100.00%> (ø)

Comment thread src/OpenTelemetry.Instrumentation.SqlClient/README.md Outdated

@cijothomas cijothomas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposed names look better.
Not explicitly approving, as pr is still marked Draft.

@mbakalov
mbakalov marked this pull request as ready for review January 12, 2021 11:55
@mbakalov
mbakalov requested a review from a team January 12, 2021 11:55
@mbakalov

Copy link
Copy Markdown
Contributor Author

@cijothomas - thank you for looking at it! Sorry, I've been away from this for a couple of days, but made a few updates now.

@cijothomas
cijothomas merged commit 97ed5f5 into open-telemetry:master Jan 20, 2021
@mbakalov
mbakalov deleted the sqlclient-statementapi branch February 26, 2021 03:17
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.

3 participants