-
Notifications
You must be signed in to change notification settings - Fork 207
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
Add Semantic definitions for Oracle Database #1911
base: main
Are you sure you want to change the base?
Conversation
* - Add semantic conventions for oracle database
schema-next.yaml
Outdated
changes: | ||
# https://github.com/open-telemetry/semantic-conventions/pull/TODO | ||
# add semantic conventions for Oracle Database | ||
- add_attributes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this file supports an add_attributes field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I have removed it now. I have added oracle specific attributes for yaml format , model/oracledb/registry.yaml
Is this ok?
(`db.operation.parameter.<key>`) in generic way.
docs/attributes-registry/README.md
Outdated
@@ -82,6 +82,7 @@ Currently, the following namespaces exist: | |||
- [NodeJS](nodejs.md) | |||
- [OCI](oci.md) | |||
- [OpenTracing](opentracing.md) | |||
- [Oracledb](oracledb.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add acronym OracleDB
to
acronyms: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed Oracledb
to OracleDB
in docs/attributes-registry/README.md
.
Also added Oracledb
in semantic-conventions/templates/registry/markdown/weaver.yaml
model/oracledb/registry.yaml
Outdated
brief: > | ||
This section defines attributes for Oracle Database. | ||
attributes: | ||
- id: oracledb.implicit_release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we avoid adding attributes without referencing them on the spans, metrics, events, resources, etc.
Is there a span/metrics that would use this attribute? Could you define it?
Otherwise, let's postpone adding this and other attributes until we're ready to define signals they should be set on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this attribute oracle.db.client.connection.implicit_release
will appear in span, when a query is executed on the connection and an implicit release of connection is done. It has only value true
and if there is no implicit release of connection, it won't appear in the span.
node-oracledb @6.7.0 onwards has code to add this information passed to OT module to add it in span.
I added the attribute in model/database/spans.yaml
.Please suggest if this looks fine?
model/oracledb/registry.yaml
Outdated
brief: > | ||
This section defines attributes for Oracle Database. | ||
attributes: | ||
- id: oracledb.implicit_release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
system-specific attribute namespace needs to match the db.system.name
identifier (i.e. it has to be oracle.db
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes modified oracledb.implicit_release
to oracle.db.client.connection.implicit_release
similarly changed other attributes.
oracle.db.client.connection.increment
oracle.db.instance
oracle.db.pdb_name
model/oracledb/registry.yaml
Outdated
brief: > | ||
The number of connections added when the pool needs to expand. | ||
examples: 5 | ||
- id: oracledb.pool.max |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please check out DB connection pool metrics - https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-metrics.md#connection-pools and check if anything oracle-specific is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the link. I removed oracledb.pool.max
and oracledb.pool.min
as they are specified in standard as
db.client.connection.max
and db.client.connection.idle.min
respectively.
I was adding these parameters in span when application creates a pool with the pool configuration (max, min ,..). I think the standard talks about this attribute to be included in metrics as UpDownCounter
, Can I still keep the same name in spans . Please correct my understanding.
Co-authored-by: Liudmila Molkova <limolkova@microsoft.com>
| [`db.query.text`](/docs/attributes-registry/db.md) | string | The database query being executed. [9] | `SELECT * FROM wuser_table where username = :mykey` | `Recommended` [10] |  | | ||
| [`db.response.returned_rows`](/docs/attributes-registry/db.md) | int | Number of rows returned by the operation. | `10`; `30`; `1000` | `Recommended` |  | | ||
| [`oracle.db.client.connection.implicit_release`](/docs/attributes-registry/oracledb.md) | boolean | Boolean flag Indicating if the internal connection is released back to pool or not after executing a query. [11] | `true` | `Recommended` |  | | ||
| [`oracle.db.client.connection.increment`](/docs/attributes-registry/oracledb.md) | int | The number of connections added when the pool needs to expand. | `5` | `Recommended` |  | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this information available on the spans or related to db query spans? DB spans describe logical operation not a transport call and would cover multiple tries (if the client retried).
Having connection-pool details on these spans sounds
- hard or impossible - these spans are create at the API surface and connection management is usually done somewhere deep
- not strictly relevant. The fact that 5 connections were added to the pool does not tell anything about this operation. Given that spans are usually sampled, it won't provide any useful insights into connection pool config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the configuration information passed for createPool
API to create a connection pool. It is only dumped in span when this API oracledb.createPool
is called. It is never dumped as part of executing an query (connection.execute('select from dual')
). So its not part of db query spans.
just to clarify with an example of application calling createPool
config = {
user: "demo",
password: "demo",
connectString: "ip:1521/freepdb1",
poolMin: 5,
poolMax: 50,
poolIncrement: 5,
}
pool = await oracledb.createPool(config);
console.log(spans[0].attributes['oracle.db.client.connection.increment']);
// 5
As i understand from your inputs, adding such configuration in db spans will not make much sense as its not indicating any logical DB operation. Hence you recommend to remove it?
probably this can just be part of application logging. Other pool configuration , poolMax (db.client.connection.max
) and poolMin(db.client.connection.idle.min
) will anyway be part of metrics.
| [`db.response.returned_rows`](/docs/attributes-registry/db.md) | int | Number of rows returned by the operation. | `10`; `30`; `1000` | `Recommended` |  | | ||
| [`oracle.db.client.connection.implicit_release`](/docs/attributes-registry/oracledb.md) | boolean | Boolean flag Indicating if the internal connection is released back to pool or not after executing a query. [11] | `true` | `Recommended` |  | | ||
| [`oracle.db.client.connection.increment`](/docs/attributes-registry/oracledb.md) | int | The number of connections added when the pool needs to expand. | `5` | `Recommended` |  | | ||
| [`oracle.db.instance`](/docs/attributes-registry/oracledb.md) | string | The name of the Oracle Database instance. | `ORCL1` | `Recommended` |  | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe oracle instance id identifies a database within a specific server?
We made a choice to put this information inside db.namespace
, see mssql conventions as an example.
So the suggestion here would be to avoid adding a new attribute and modifying namespace description to include instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I understand, your suggestion is to combine instance name
and pdb name
with db.namespace
(it only has service name now).
The service name
is the input given by user for getting a connection along with hostname
, port
.
Ex:
const configSV1 = "hostname:port/servicename" // configuration for getting a connection to database.
const conn1 = await oracledb.getConnection(configSV1) // get first connection to database, It can connect to instance1.
const conn2 = await oracledb.getConnection(configSV1) // get second connection to database, It can connect to instance2.
The instance name
and pdb name
are filled after a successful connection. So it is possible that conn1
can get a different instance name
from conn2
.
Will this be an issue, If db.namespace
includes additional details (runtime detected) like instance name
and pdb name
for the same user input, configSV1
like co-relating spans,.. ?
| [`oracle.db.client.connection.implicit_release`](/docs/attributes-registry/oracledb.md) | boolean | Boolean flag Indicating if the internal connection is released back to pool or not after executing a query. [11] | `true` | `Recommended` |  | | ||
| [`oracle.db.client.connection.increment`](/docs/attributes-registry/oracledb.md) | int | The number of connections added when the pool needs to expand. | `5` | `Recommended` |  | | ||
| [`oracle.db.instance`](/docs/attributes-registry/oracledb.md) | string | The name of the Oracle Database instance. | `ORCL1` | `Recommended` |  | | ||
| [`oracle.db.pdb_name`](/docs/attributes-registry/oracledb.md) | string | The name of the pluggable database (PDB) the connection is using. | `PDB1` | `Recommended` |  | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it sounds like another component of DB namespace, could it be incorporated there?
| [`db.query.summary`](/docs/attributes-registry/db.md) | string | Low cardinality representation of a database query text. [7] | `SELECT wuser_table`; `INSERT shipping_details SELECT orders`; `get user by id` | `Recommended` [8] |  | | ||
| [`db.query.text`](/docs/attributes-registry/db.md) | string | The database query being executed. [9] | `SELECT * FROM wuser_table where username = :mykey` | `Recommended` [10] |  | | ||
| [`db.response.returned_rows`](/docs/attributes-registry/db.md) | int | Number of rows returned by the operation. | `10`; `30`; `1000` | `Recommended` |  | | ||
| [`oracle.db.client.connection.implicit_release`](/docs/attributes-registry/oracledb.md) | boolean | Boolean flag Indicating if the internal connection is released back to pool or not after executing a query. [11] | `true` | `Recommended` |  | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if that's possible to add it to DB spans. E.g. if I take a JDBC example:
public static void viewTable() throws SQLException {
String query = "select COF_NAME, SUP_ID, PRICE, SALES, TOTAL from COFFEES";
try (Connection conn = odsconn.getConnection()) {
try (Statement stmt = con.createStatement()) {
ResultSet rs = stmt.executeQuery(query);
// this is where SQL span ends
while (rs.next()) {
...
}
} catch (SQLException e) {
}
}
} // this where the connection is returned to the pool
it seems, DB span will be ended by the time connection is closed.
Let me know if I'm missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As part of stmt.executeQuery(query);
, the oracledb
driver adds a span attribute , oracle.db.client.connection.implicit_release = true
, if the completion of the query execution can implicitly release the connection back to pool. Hence this flag, implicit_release
is detected at end of reading the query response from server.
Probably I should rename this oracle.db.client.connection.implicit_release to
oracle.db.query.implicit_release` ?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As part of
stmt.executeQuery(query);
, theoracledb
driver adds a span attribute ,oracle.db.client.connection.implicit_release = true
, if the completion of the query execution can implicitly release the connection back to pool. Hence this flag,implicit_release
is detected at end of reading the query response from server.Probably I should rename this
oracle.db.client.connection.implicit_release to
oracle.db.query.implicit_release` ?.
just to add on this, the server sends the information that an implicit release is done , only if there are no more rows (there are more other conditions to be satisfied) to be fetched from application, So server can send this flag as part of rs.next()
call too. I hope i could clarify.
Fixes #
Changes
Add semantic conventions for Oracle Database.
Note: if the PR is touching an area that is not listed in the existing areas, or the area does not have sufficient domain experts coverage, the PR might be tagged as experts needed and move slowly until experts are identified.
Merge requirement checklist
[chore]