Update database instrumentations to follow semantic conventions #1354
Conversation
srikanthccv
requested review from
a team,
owais and
toumorokoshi
and removed request for
a team
| @@ -106,7 +108,7 @@ def succeeded(self, event: monitoring.CommandSucceededEvent): | |||
| return | |||
| if span.is_recording(): | |||
| span.set_attribute( | |||
| "db.mongo.duration_micros", event.duration_micros | |||
| "db.mongodb.duration_micros", event.duration_micros | |||
There was a problem hiding this comment.
looks like this isn't present in semantic conventions. Should we add it?
There was a problem hiding this comment.
Spec has call level attributes for specific technologies. It is left to authors of db package to decide whether they want to add it or not. For example one would be interested in number of rows returned by query etc..
There was a problem hiding this comment.
There are also language specific attributes, and duration_micros is not one of them.
To clarify I'm not arguing against attributes for specific technologies. I'm saying I think this is adding attributes that aren't in the spec. So it seems like we should send a PR to the spec too.
There was a problem hiding this comment.
Agree. I think the rationale behind including them is that pymongo monitoring module provides handful of attributes such as operation_id, request_id, duration_micros, command_name and few others and it makes sense to include them with PymongoInstrumentor by default.
I am not really sure about sending PR to spec though.
There was a problem hiding this comment.
@lzchen , I would appreciate your comments on this before opening PR in contrib repo.
| self.assertEqual(span.attributes["db.mongo.sort"], "sort") | ||
| self.assertEqual(span.attributes["db.mongo.limit"], "limit") | ||
| self.assertEqual(span.attributes["db.mongo.pipeline"], "pipeline") | ||
| self.assertEqual(span.attributes["db.mongodb.filter"], "filter") |
There was a problem hiding this comment.
I don't see these in semantic conventions as well. What should these be?
There was a problem hiding this comment.
Same as above. If a database package adds a call level attributes to specific tech, it should be in the format of db.{DBMS}.{attr}:{val}.
There was a problem hiding this comment.
MongoDB has an interesting way of building the query, relying on function calls rather than a string containing the full statement.
The specification has a convention for the statement, so if we followed the spec it may make more sense to set the raw query in the string instead, rather than put the sub-components of the statement in individual attributes.
codeboten
added
the
instrumentation
|
@lonewolf3739 |
Addresses #1298