Add yaml semantic conventions for Database #928
Conversation
| **Additional attribute requirements:** At least one of the following sets of attributes is required: | ||
|
|
||
| * [`net.peer.name`](span-general.md) | ||
| * [`net.peer.ip`](span-general.md) |
There was a problem hiding this comment.
Strictly speaking, this single attribute is not a set, at least not in that notation. Should we add surrounding parentheses in the MD generator or is it clear enough that it is either the first or the second line (not both together as one set)?
There was a problem hiding this comment.
+1 to @arminru. I am confused by the reference to "sets".
There was a problem hiding this comment.
Would the phrase "At least one of the following is required:" be a good replacement?
There was a problem hiding this comment.
If we then add "All of:" to each set with more than one attribute (we have none here, but for example in the HTTP conventions), that would be a good improvement I think.
| type: string | ||
| brief: > | ||
| Username for accessing the database. | ||
| examples: ['readonly_user', 'reporting_user'] |
There was a problem hiding this comment.
It is not completely obvious in the resulting markdown that these are 2 distinct examples. It was clear in the original markdown. Perhaps remove one of the examples to avoid confusion.
| tag: connection-level | ||
| required: | ||
| conditional: See below. | ||
| - ref: net.peer.ip |
There was a problem hiding this comment.
Being able to reference other attributes is very nice!
| **Additional attribute requirements:** At least one of the following sets of attributes is required: | ||
|
|
||
| * [`net.peer.name`](span-general.md) | ||
| * [`net.peer.ip`](span-general.md) |
There was a problem hiding this comment.
+1 to @arminru. I am confused by the reference to "sets".
| <!-- semconv db.mssql(tag=connection-level-tech-specific,remove_constraints) --> | ||
| | Attribute | Type | Description | Example | Required | | ||
| |---|---|---|---|---| | ||
| | `db.jdbc.driver_classname` | string | The fully-qualified class name of the [Java Database Connectivity (JDBC)](https://docs.oracle.com/javase/8/docs/technotes/guides/jdbc/) driver used to connect. | `org.postgresql.Driver`<br>`com.microsoft.sqlserver.jdbc.SQLServerDriver` | No | |
There was a problem hiding this comment.
The example is confusing, not clear that it is 2 examples since I can just imagine it one line that is wrapped in a table cell.
Either use one example or enumerate them somehow to make it clear.
There was a problem hiding this comment.
this is not new to this document. My suggestion would be to address it in different PR.
There was a problem hiding this comment.
In the rendered view it is displayed more nicely:
We could update the markdown generator to add a comma in between, to make it even clearer. That would be a separate issue in https://github.com/open-telemetry/build-tools/, however.
There was a problem hiding this comment.
The example is confusing
I think with the new generator, you should either read the YAML source or the rendered markdown. The raw, unrendered markdown tables were barely readable already previously, and with the addition of the type column, its just unreadable IMHO. But that's OK, that's why we use a generator.
There was a problem hiding this comment.
Just to make it clear I meant: it is rendered differently by the generator. In the original we had this:
in the new generated version we have this:
I think the usage of "or" in the original version makes it clear that these 2 examples. In the new version it is not so apparent even though they are on 2 separate lines.
We can probably try to improve the generator to add an "or" between examples. Doesn't have to be done in this PR, can be improved in the future. Looks good otherwise to me.
There was a problem hiding this comment.
I've created open-telemetry/build-tools#8 to track these changes :)
| tag: call-level | ||
| type: string | ||
| required: | ||
| conditional: Required, if `db.statement` is not applicable. |
There was a problem hiding this comment.
would be cool to be able to use attribute reference in descriptions as well =)
There was a problem hiding this comment.
Yeah, also for generated Javadoc, etc. But that's a rather complex feature as we would need to parse the markdown in the tool then.
There was a problem hiding this comment.
beyond small comments from @arminru and @tigrannajaryan it looks good.
Oberon00
added
area:semantic-conventions
| <!-- semconv db.mssql(tag=connection-level-tech-specific,remove_constraints) --> | ||
| | Attribute | Type | Description | Example | Required | | ||
| |---|---|---|---|---| | ||
| | `db.jdbc.driver_classname` | string | The fully-qualified class name of the [Java Database Connectivity (JDBC)](https://docs.oracle.com/javase/8/docs/technotes/guides/jdbc/) driver used to connect. | `org.postgresql.Driver`<br>`com.microsoft.sqlserver.jdbc.SQLServerDriver` | No | |
There was a problem hiding this comment.
Just to make it clear I meant: it is rendered differently by the generator. In the original we had this:
in the new generated version we have this:
I think the usage of "or" in the original version makes it clear that these 2 examples. In the new version it is not so apparent even though they are on 2 separate lines.
We can probably try to improve the generator to add an "or" between examples. Doesn't have to be done in this PR, can be improved in the future. Looks good otherwise to me.
|
please rebase so I can merge this PR. |
* Add yaml semantic conventions for Database * Address feedback
Changes
Updated Database semantic convention to the YAML format.
Related issues #547