feat(db.values): add db.bind.values to yaml

[haddasbronfman]

Fixes open-telemetry/semantic-conventions#716

Changes

Add db.bind.values to DB span. it contains the values that come with the query (like in pg/mysql and more)

[haddasbronfman]

@joaopgrassi This is the "real" PR. I closed the other one (#3091 )

[joaopgrassi]

Also, please add a change log entry for this change under the Semantic Conventions section:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md?plain=1#L39

[Flarna]

What about using db.bind.values of types string[] and additionally db.bind.keys of type string[]?
Or a bit longer db.bind_params.keys/db.bind_params.values?

If there are no bind keys it should be omitted, if they exist the size of the two arrays must match and key[x] fits to value[x].

[joaopgrassi]

We can discuss about naming, but I think capturing the keys also makes sense. Then the bind_values requirement level can be Conditionally Required based if the keys are also recorded and they must match in index as you said.

What do you think @haddasbronfman?

[joaopgrassi]

I guess this can also be done in a follow-up PR.

[Flarna]

Adding more attributes in a followup should be fine but I think we should avoid renames in followups. In special the decision to use a namespace (db.bind.XXX) or not not should happen now.

Or is there a specific reason to merge fast? Once merge instrumentations may use it and if we rename this creates unneeded friction.

[haddasbronfman]

do you mean something like this? db. bind_params.keys = [name, age, city] and db. bind_params.values = [David, 38, NY] ?
Just to make sure I understood you.

I'm not sure if this is necessary because the db.statement attribute already gives us the keys of the query (e.g. "select * from TABLE where name=? and age=? and city=?")

[blumamir]

It makes sense to me to have the "db.bind" namespace and store arrays under this namespace. if I understand correctly, the suggestion is to spec "db.bind.values" now and leave the other attributes to follow-up PRs, right?

[joaopgrassi]

Saw your comment below now. I think the idea was not to introduce a new namespace.. but maybe we should? Not sure now 🤔

[haddasbronfman]

So I think the only thing left is to decide whether to use db.bind_values or db.bind.values. @joaopgrassi What do you think after what @blumamir wrote below?

[joaopgrassi]

I don't have a strong opinion on it.. as long as the namespace doesn't collide later with something "standalone" and has an actual need to be the home of multiple attributes then I think it makes sense.

[arminru]

I think it makes sense as it would allow us to introduce db.bind.keys and db.bind.types in future.

[joaopgrassi]

@haddasbronfman are you aware of instrumentation recording such values already? Would be interesting to know and could help us define things here.

[haddasbronfman]

Hi, yes.
I added it in mysql and it exists in postgres too.

[joaopgrassi]

I added it in mysql and it exists in postgres too.

@haddasbronfman thanks for the links. I think this looks good. It's still experimental, so if when updating the implementations something comes up we can always tweak it. Also about the topic on recording the "bind keys"

[blumamir]

Another example supporting the db.bind namespace: aws dyanmodb has many places in the API where user needs to specify the "type" of the bind values. For example:

const params = {
  // Specify which items in the results are returned.
  FilterExpression: "Subtitle = :topic AND Season = :s AND Episode = :e",
  // Define the expression attribute value, which are substitutes for the values you want to compare.
  ExpressionAttributeValues: {
    ":topic": {S: "SubTitle2"},
    ":s": {N: 1},
    ":e": {N: 2},
  },
  // Set the projection expression, which are the attributes that you want.
  ProjectionExpression: "Season, Episode, Title, Subtitle",
  TableName: "EPISODES_TABLE",
};

ddb.scan(params, function (err, data) {
  if (err) {
    console.log("Error", err);
  } else {
    console.log("Success", data);
    data.Items.forEach(function (element, index, array) {
      console.log(
          "printing",
          element.Title.S + " (" + element.Subtitle.S + ")"
      );
    });
  }
});

Here, for each bind value we have: "value", "key" (or name) and type. I imagine that storing those arrays into a single semantic attribute namespace could be helpful

[haddasbronfman]

According to the suggestions that came up here, I changed the name to db.bind.values. Please approve @joaopgrassi joaopgrassi, @arminru, @Flarna, @blumamir

[blumamir]

As discussed in this PR, the attribute can also be used for non-SQL statements. Do you think we should update the text accordingly?

[blumamir]

Suggested change

	An array with the values of the SQL statement bind parameters.
	An array with the values of the db statement bind parameters.

or:

Suggested change

	An array with the values of the SQL statement bind parameters.
	An array with the values of the db operation bind parameters.

?

[jsuereth]

I have a few concerns with this PR:

1. It seems to only apply to positional templates. Specifically, something like SELECT * FROM my_table WHERE my_column = ? AND other_column = ?. What about "named" parameters where you have SELECT * FROM my_table WHERE my_column = ?some_key??
2. Is it implicit that the "template" SQL is the sql statement or do we need to put that SQL statement in the same "bind" namespace?
3. Should we put caveats about leaked PII or sensitive data in these fields and ask instrumentation others to provide on/off signals for this data? This is likely a more GENERAL concern for instrumentation that need to be handled outside the scope of this PR.

[haddasbronfman]

1. you're right. this PR covers only the first concern (a query with '?' in it). For the second concern (a query with "named" parameters) there is an issue one of my colleague opened for that: DB sanitization uniform format semantic-conventions#717.
   As far as I know, currently, we don't edit/change/sanitize queries. But for the case that a query comes with accompanying parameters - we want to let the user have the option to see them in the span.

2. (If I understood you correctly) Yes. the "templated" query is in the db.statement. (e.g db.statement: select...where column = ?...). It won't be in the db.dinb namespace.

3. As I mentioned in 1 - there is an issue in the specification that tries to answer this concert.

[blumamir]

1. you're right. this PR covers only the first concern (a query with '?' in it). For the second concern (a query with "named" parameters) there is an issue one of my colleague opened for that: DB sanitization uniform format #3150.

I support making this explicit in the current proposal (adding the text "An array with the values for a statement positional bind parameters."), or adding the db.bind.keys and describe the behavior for both "positional" and "named" parameters.

2. Is it implicit that the "template" SQL is the sql statement or do we need to put that SQL statement in the same "bind" namespace?

IMO, it might be confusing to have the statement in the db.bind. namespace. It could have been all hosted under the db.statement namespace, e.g. db.statmenet.values but then the db.statement itself cannot be used to store a string value which can be painful to change at this point.

[haddasbronfman]

IMO, it might be confusing to have the statement in the db.bind. namespace. It could have been all hosted under the db.statement namespace, e.g. db.statmenet.values but then the db.statement itself cannot be used to store a string value which can be painful to change at this point.

I think it is better to keep db.statement as is because the overhead of changing its name can be very unexpected (although we change it only in the spec, It still appears in the SDKs, and docs... it would take a long time to rename it).

[haddasbronfman]

@jsuereth Consulting with Amir we thought we should clearly write that the db.bind.values is only for positional templates. What about one of the following options? :

• "An array with the values of the SQL statement positional bind parameters"
• "An array with the values of the SQL statement bind parameters. relevant/applies only to positional templates.

Given the discussion/direction changed, I'm removing my approval for now

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.