Adapt Elasticsearch spec to stability changes #1002
Conversation
gregkalapos
changed the title
gregkalapos
changed the title
| brief: The index or data stream against which the query is executed. | ||
| note: > | ||
| If the query targets multiple indices or data streams, then the name of those should be added as a comma separated list. | ||
| If the query doesn't target a specific index, this field should remain empty. |
There was a problem hiding this comment.
| If the query doesn't target a specific index, this field should remain empty. | |
| If the query doesn't target a specific index, this field MUST NOT be set. |
| @@ -14,12 +14,13 @@ described on this page. | |||
|
|
|||
| ## Span Name | |||
|
|
|||
| The **span name** SHOULD be of the format `<endpoint id>`. | |||
| The **span name** SHOULD be of the format `{db.operation.name} {db.collection.name}`. | |||
There was a problem hiding this comment.
I wonder if this section is necessary, it seems to be perfectly aligned with the general db span name guidance now.
There was a problem hiding this comment.
Yeah, that's a good point, but a bit tricky as well. In general what I propose here is very similar to the general db span name guideline, but it differs a bit, specifically: db.collection.name isn't available for all queries - there could be queries that target all indices or no index - in that case we won't have this value and it'd be very unfortunate to fall back to the next item in the {target} list from the general spec. I think the target list is very useful when all spans have the same set of attributes, but if the list of attributes changes within the same instrumentation (and potentially within the same trace), then we need a solution for that, otherwise the span name format will be mixed.
I opened #1045 to discuss this.
There was a problem hiding this comment.
Opened #1061 to address it. If that goes in, we can drop this part and Elasticsearch can follow the general DB span name guidelines.
| | [`db.query.text`](/docs/attributes-registry/db.md) | string | The request body for a [search-type query](https://www.elastic.co/guide/en/elasticsearch/reference/current/search.html), as a json string. | `"{\"query\":{\"term\":{\"user.id\":\"kimchy\"}}}"` | `Recommended` [9] |  | | ||
| | [`network.peer.address`](/docs/attributes-registry/network.md) | string | Peer address of the database node where the operation was performed. [10] | `10.1.2.80`; `/tmp/my.sock` | `Recommended` |  | | ||
| | [`db.collection.name`](/docs/attributes-registry/db.md) | string | The index or data stream against which the query is executed. [7] | `my_index`; `index1, index2` | `Recommended` |  | | ||
| | [`db.elasticsearch.cluster.name`](/docs/attributes-registry/db.md) | string | Represents the identifier of an Elasticsearch cluster. | `e9106fc68e3044f0b1475b04bf4ffd5f` | `Recommended` [8] |  | |
There was a problem hiding this comment.
can this be replaced by db.namespace?
Follow up from #974 (comment).
Changes
db.collection.nameto the Elasticsearch spec, which is used in the span name as well.http.request.methodtodb.system. The thinking behind this is thatdb.systemis usually the last fallback for other DBs. This was also the outcome in Better guidance on semantic conventions for database client call span names in case of missing information #704. In practice this fallback is used extremely rarely since endpoint id is usually available.Questions
db.elasticsearch.cluster.namecould be replaced withdb.namespace. Do we want to do that?Merge requirement checklist
[chore]