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

Database semantic conventions: Add db.sql.table, allow SQL keyword as db.operation #1141

Merged
merged 7 commits into from
Nov 9, 2020

Conversation

justinfoote
Copy link
Member

Fixes #1104

Changes

This PR adds a new attribute to the trace semantic conventions for database operations: db.table, and adds guidance that this attribute should be set only if the value is readily available from the library being instrumented.
It also changes the guidance around db.operation to indicate that it should be set to the SQL keyword, but also only if the value is readily available from the library being instrumented.

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Table is important, this seems nice

specification/trace/semantic_conventions/database.md Outdated Show resolved Hide resolved
It is not recommended to attempt any client-side parsing of
`db.statement` just to get this property, but it should be set if
the operation name is provided by the library being instrumented.
If the statement has an ambiguous operation, or performs more than
Copy link
Member

Choose a reason for hiding this comment

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

This seems to imply that operation is always derived from statement, which is not true. In fact, the main purpose of this attribute is to support DBs that do not use a statement/query language as their API, such as CouchDB.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I see your point. This certainly was not my intention.

In the main "brief" description of this attribute, we still list the mongo example first. We could add a few more examples there.

Or I wonder if it would be better to include something in the footnote like this:

note: >
    In the case of SQL databases, the operation should be set to the SQL keyword
    (example: `SELECT` or `INSERT`). 
    It is not recommended to attempt any client-side parsing of
    `db.statement` just to get this property, but it should be set if 
    the operation name is provided by the library being instrumented.
    If the statement has an ambiguous operation, or performs more than
    one operation, this value may be omitted.

Is this redundant with the note in the 'required' column: "Required, if db.statement is not available."

What do you think?

Copy link
Member

@Oberon00 Oberon00 Oct 30, 2020

Choose a reason for hiding this comment

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

I suggested a simpler change by emphasizing that that part only applies to SQL in #1141 (review)

@justinfoote
Copy link
Member Author

@open-telemetry/technical-committee Is this ready to merge?

semantic_conventions/trace/database.yaml Outdated Show resolved Hide resolved
semantic_conventions/trace/database.yaml Outdated Show resolved Hide resolved
It is not recommended to attempt any client-side parsing of
`db.statement` just to get this property, but it should be set if
the operation name is provided by the library being instrumented.
If the statement has an ambiguous operation, or performs more than
Copy link
Member

@Oberon00 Oberon00 Oct 30, 2020

Choose a reason for hiding this comment

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

I suggested a simpler change by emphasizing that that part only applies to SQL in #1141 (review)

@Oberon00 Oberon00 changed the title Add db.table to database semantic conventions Database semantic conventions: Add db.table, allow SQL keyword as db.operation Oct 30, 2020
If the statement has an ambiguous operation, or performs more than
one operation, this value may be omitted.
examples: ['findAndModify', 'HMSET', 'SELECT']
- id: table
Copy link
Member

Choose a reason for hiding this comment

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

Actually I just realized that the term table is probably specific to SQL. Thus I recommend adding this to a new semantic convention db.sql in this file that extends: db and has this table attribute, so that the full name is db.sql.table.

Copy link
Member

Choose a reason for hiding this comment

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

An alternative might be to have a more generic term, but since a table likely has no 1:1 mapping to other DB concepts (there might be several concepts that match a bit), I think having this in a SQL-specific convention would be easier.

Copy link
Member Author

@justinfoote justinfoote Oct 30, 2020

Choose a reason for hiding this comment

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

This is a good idea. I'll leave the operation where it is, but move the table to an SQL specific section in Call-level attributes for specific technologies.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 657670c

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

Requesting changes because I have concerns for adding this SQL-specific attribute to the very general DB conventions.

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for responding to my comments so quickly!

@justinfoote
Copy link
Member Author

@open-telemetry/technical-committee I think this actually is ready to merge now.

semantic_conventions/trace/database.yaml Outdated Show resolved Hide resolved
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
@justinfoote
Copy link
Member Author

@bogdandrutu @jmacd -- this is the PR I mentioned in the metrics SIG today that's holding up the database metrics semantic conventions.

@arminru
Copy link
Member

arminru commented Nov 9, 2020

@justinfoote Sorry I've missed that before: Could you please add an entry in the changelog to make people aware of the new table and the changed operation attribute? Once that's done we can merge this PR right away.

@justinfoote
Copy link
Member Author

@arminru Added to the changelog in 862361f.

@arminru arminru merged commit 1954360 into open-telemetry:master Nov 9, 2020
@justinfoote justinfoote deleted the db_table_semantics branch November 9, 2020 21:33
@jamal
Copy link

jamal commented Nov 10, 2020

One question about this change, I noticed that we still have db.mongodb.collection. Is it expected to use both or should the other be deprecated in favor of db.table?

@arminru
Copy link
Member

arminru commented Nov 10, 2020

@jamal Actually we should have updated the PR name and changelog entry - it is called db.sql.table now and explicitly only applies to SQL-like databases and is thus not a replacement for the dedicated db.mongodb.collection.

@arminru arminru changed the title Database semantic conventions: Add db.table, allow SQL keyword as db.operation Database semantic conventions: Add db.sql.table, allow SQL keyword as db.operation Nov 10, 2020
arminru added a commit to dynatrace-oss-contrib/opentelemetry-specification that referenced this pull request Nov 10, 2020
arminru added a commit that referenced this pull request Nov 10, 2020
* Fix changelog entry for #1141

* Apply suggestions from code review

Co-authored-by: Christian Neumüller <christian.neumueller@dynatrace.com>
@jamal
Copy link

jamal commented Nov 10, 2020

@arminru Ah, I missed that, thank you. This definitely solves some of the problems I wanted to solve.

An aside but being new to all of this, I'm a bit curious. Was this discussed in a specification meeting? I don't see any conversations around this in the PR, and I would have loved to be able to provide some feedback.

Having a specific solution for sql makes sense, since table is not a generic name (for example, Mongo). But, there are cases I wanted to cover as well that don't make sense. For example, Redis doesn't have a concept of table / collection. But, in our own implementation we introduced a wrapper over Lettuce to allow us to specify a collection name at the connection level. This allows us to segment metrics / traces, and I was hoping to extend our own instrumentation to tag this in traces. Obviously, this is not standard so it wouldn't be part of the spec.

But, because this change is specific to SQL, vendors / backends may not support this for Redis and allow us to provide this enhancement. I wonder if there are any thoughts on how to support cases like this?

I'll review other cases to see if I can think of other gaps I would want to make sure are covered and will open a separate issue if it comes up.

Thanks so much!

@Oberon00
Copy link
Member

Oberon00 commented Nov 10, 2020

Was this discussed in a specification meeting? I don't see any conversations around this in the PR, and I would have loved to be able to provide some feedback.

There is more discussion in the linked issue #1104. I don't remember this being discussed in a meeting.

@jamal
Copy link

jamal commented Nov 10, 2020

@Oberon00 Oh, sorry, I feel silly. Totally missed that conversation when I checked the Issue again. Thanks! I'll think through this and will start a separate conversation if I have any recommendations on how this could be extended. Thanks so much!

carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
…operation (open-telemetry#1141)

* Add db.table to database semantic conventions

* Fix spacing in db semantic conventions

* Move db.table to SQL-specific call level attributes

* Clarify instructions for setting db.operation to a SQL keyword

* Specify that db.sql.table MUST NOT be set when not applicable

Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

* Add db.table and db.operation to CHANGELOG

Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
* Fix changelog entry for open-telemetry#1141

* Apply suggestions from code review

Co-authored-by: Christian Neumüller <christian.neumueller@dynatrace.com>
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.

Database table / collection semantic conventions
9 participants