Skip to content

Implement some of the stable database semantic conventions#11575

Merged
trask merged 95 commits intoopen-telemetry:mainfrom
heyams:heya/implement-database-stable-semconv
Oct 29, 2024
Merged

Implement some of the stable database semantic conventions#11575
trask merged 95 commits intoopen-telemetry:mainfrom
heyams:heya/implement-database-stable-semconv

Conversation

@heyams
Copy link
Copy Markdown
Contributor

@heyams heyams commented Jun 13, 2024 •
edited by trask
Loading

@github-actions github-actions Bot added the test native This label can be applied to PRs to trigger them to run native tests label Jun 13, 2024
@heyams heyams mentioned this pull request Aug 27, 2024
Closed
@heyams heyams marked this pull request as ready for review August 30, 2024 16:24
@heyams heyams requested a review from a team August 30, 2024 16:24
trask
trask reviewed Sep 1, 2024
View reviewed changes
Comment thread .../io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlClientAttributesExtractor.java Outdated
Comment thread ...o/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientCommonAttributesGetter.java
Comment thread ...o/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientCommonAttributesGetter.java
Comment thread ...a/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientAttributesExtractor.java Outdated
Comment thread ...a/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientAttributesExtractor.java Outdated
Comment thread ...ava/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlClientAttributesGetter.java Outdated
Comment thread ...c/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/rpc/RpcMetricsAdvice.java
@heyams
Copy link
Copy Markdown
Contributor Author

heyams commented Sep 13, 2024

I will add 'error.type' and the opt-in db.query.parameter.<key> in a follow-up PR.

trask
trask reviewed Sep 16, 2024
View reviewed changes
Comment thread ...java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientAttributesGetter.java Outdated
Comment thread ...java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientAttributesGetter.java Outdated
@trask trask mentioned this pull request Sep 24, 2024
Closed
@heyams heyams requested a review from a team as a code owner September 30, 2024 19:22
trask
trask reviewed Oct 1, 2024
View reviewed changes
Comment thread ...java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientAttributesGetter.java
Comment thread ...java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientAttributesGetter.java
Comment thread ...pentelemetry/instrumentation/api/incubator/semconv/db/DbClientCommonAttributesExtractor.java
Comment thread ...o/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientCommonAttributesGetter.java
Comment thread ...o/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientCommonAttributesGetter.java
Comment thread ...ava/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientSpanNameExtractor.java Outdated
Comment thread ...ava/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientSpanNameExtractor.java Outdated
Comment thread .../io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlClientAttributesExtractor.java Outdated
trask
trask reviewed Oct 1, 2024
View reviewed changes
Comment thread ...ava/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientSpanNameExtractor.java Outdated
Comment thread ...ava/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlClientAttributesGetter.java
Comment thread ...on/src/main/java/io/opentelemetry/instrumentation/testing/junit/db/SemconvStabilityUtil.java Outdated
Comment thread ...in/java/io/opentelemetry/javaagent/instrumentation/vertx/v4_0/sql/VertxSqlClientRequest.java Outdated
Comment thread ...entelemetry/javaagent/instrumentation/vertx/v4_0/redis/VertxRedisClientAttributesGetter.java
Comment thread ...ain/java/io/opentelemetry/javaagent/instrumentation/rediscala/RediscalaAttributesGetter.java
Comment thread ...main/java/io/opentelemetry/instrumentation/r2dbc/v1_0/internal/R2dbcSqlAttributesGetter.java
Comment thread .../library/src/main/java/io/opentelemetry/instrumentation/r2dbc/v1_0/internal/DbExecution.java Outdated
Comment thread .../java/io/opentelemetry/javaagent/instrumentation/lettuce/v5_0/LettuceDbAttributesGetter.java Outdated
Comment thread .../java/io/opentelemetry/javaagent/instrumentation/lettuce/v5_0/LettuceDbAttributesGetter.java
@heyams heyams requested a review from trask October 2, 2024 17:53
trask
trask reviewed Oct 2, 2024
View reviewed changes
Comment thread ...rc/main/java/io/opentelemetry/javaagent/instrumentation/cassandra/v4_0/CassandraRequest.java Outdated
Comment thread .../library/src/main/java/io/opentelemetry/instrumentation/cassandra/v4_4/CassandraRequest.java Outdated
Comment thread ...ion/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/DbRequest.java Outdated
Comment thread .../library/src/main/java/io/opentelemetry/instrumentation/r2dbc/v1_0/internal/DbExecution.java Outdated
Comment thread ...in/java/io/opentelemetry/javaagent/instrumentation/vertx/v4_0/sql/VertxSqlClientRequest.java Outdated
trask
trask reviewed Oct 2, 2024
View reviewed changes
Comment thread ...common/testing/src/main/java/io/opentelemetry/cassandra/v4/common/AbstractCassandraTest.java Outdated
Comment thread ...common/testing/src/main/java/io/opentelemetry/cassandra/v4/common/AbstractCassandraTest.java Outdated
Comment thread ...src/main/java/io/opentelemetry/instrumentation/cassandra/v4_4/CassandraTelemetryBuilder.java
Comment thread ...4/testing/src/main/java/io/opentelemetry/testing/cassandra/v4_4/AbstractCassandra44Test.java Outdated
Comment thread ...avaagent/instrumentation/elasticsearch/transport/ElasticsearchTransportAttributesGetter.java Outdated
Comment thread ...main/java/io/opentelemetry/javaagent/instrumentation/jedis/v3_0/JedisDbAttributesGetter.java Outdated
Comment thread ...brary/src/main/java/io/opentelemetry/instrumentation/mongo/v3_1/MongoDbAttributesGetter.java Outdated
Comment thread .../opentelemetry/javaagent/instrumentation/opensearch/rest/OpenSearchRestAttributesGetter.java Outdated
Comment thread .../library/src/main/java/io/opentelemetry/instrumentation/r2dbc/v1_0/internal/DbExecution.java Outdated
Comment thread ...in/java/io/opentelemetry/javaagent/instrumentation/vertx/v4_0/sql/VertxSqlClientRequest.java Outdated
@trask
Copy link
Copy Markdown
Member

trask commented Oct 17, 2024 •
edited
Loading

status update, still need to:

  • add the db.client.operation.duration metric
  • add the db.query.summary attribute (requires updating the jflex sql parsing)
  • update db.operation.name and db.collection.name to only capture when there's a single one (currently they are populated as the first when there are multiple)
  • upstream db.namespace definitions to semconv repo for all of the supported instrumentations
  • API changes
    • remove DbClientCommonAttributesGetter (hopefully only need DbClientAttributesGetter)
    • change DbClientAttributesGetter generics from <REQUEST> to <REQUEST, RESPONSE> (need in order for it to extend NetworkAttributesGetter but also probably to make it more future proof because of db.response.return_rows and other future semconv)
    • make DbClientAttributesGetter extend NetworkAttributesGetter (and ServerAttributesGetter?) similar to HttpClientAttributesGetter
  • add tests for semconv opt-in for the remaining database modules
    • :instrumentation:aws-sdk:aws-sdk-22
    • :instrumentation:camel-220
    • :instrumentation:clickhouse-client-05
    • :instrumentation:elasticsearch:elasticsearch-api-client-716
    • :instrumentation:elasticsearch:elasticsearch-rest-50
    • :instrumentation:elasticsearch:elasticsearch-rest-64
    • :instrumentation:elasticsearch:elasticsearch-rest-70
    • :instrumentation:elasticsearch:elasticsearch-transport-53
    • :instrumentation:elasticsearch:elasticsearch-transport-common
    • :instrumentation:geode-14
    • :instrumentation:hibernate:hibernate-33
    • :instrumentation:hibernate:hibernate-40
    • :instrumentation:hibernate:hibernate-60
    • :instrumentation:hibernate:hibernate-60:spring-
    • :instrumentation:hibernate:hibernate-procedure-call-43
    • :instrumentation:influxdb-24
    • :instrumentation:jdbc
    • :instrumentation:jedis:jedis-14
    • :instrumentation:jedis:jedis-30
    • :instrumentation:lettuce:lettuce-40
    • :instrumentation:lettuce:lettuce-50
    • :instrumentation:lettuce:lettuce-51
    • :instrumentation:mongo:mongo-common
    • :instrumentation:opensearch:opensearch-rest-10
    • :instrumentation:r2dbc-10
    • :instrumentation:rediscala-18
    • :instrumentation:spring:spring-data:spring-data-30
    • :instrumentation:spring:spring-data:spring-data-common
    • :instrumentation:spymemcached-212
    • :instrumentation:vertx:vertx-rx-java-35
    • :smoke-tests-otel-starter:spring-boot-common
    • :smoke-tests-otel-starter:spring-boot-reactive-common

@trask trask force-pushed the heya/implement-database-stable-semconv branch from 22f2903 to 038fbec Compare October 28, 2024 02:46
@trask trask changed the title Implement stable database semantic conventions Implement some of the stable database semantic conventions Oct 28, 2024
laurit
laurit reviewed Oct 28, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@laurit laurit left a comment

Choose a reason for hiding this comment

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

I think we should merge #12292 before this one (there'll probably be conflicts) so we could use the new attributes. Idk why we stopped updating the semconv artifact.

laurit
laurit reviewed Oct 28, 2024
View reviewed changes
Comment thread ...on/src/main/java/io/opentelemetry/instrumentation/testing/junit/db/SemconvStabilityUtil.java
}

@SuppressWarnings("unchecked")
public static <T> AttributeKey<T> getAttributeKey(AttributeKey<T> oldKey) {
Copy link
Copy Markdown
Contributor

@laurit laurit Oct 28, 2024

Choose a reason for hiding this comment

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

I don't remember which way we did this for the http but mapping from new to old might be preferable here as it reduces the need to suppress deprecation warnings when old attributes are used (after we update the semconv jars). For http this might not have been an issue because most of the http tests are in a base class so we needed to suppress the warnings only here.

Copy link
Copy Markdown
Member

@trask trask Oct 28, 2024

Choose a reason for hiding this comment

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

we wouldn't be able to map db.collection.name since multiple attributes were consolidated into this one attribute

    map.put(DB_SQL_TABLE, DB_COLLECTION_NAME);
    map.put(DB_CASSANDRA_TABLE, DB_COLLECTION_NAME);
    map.put(DB_MONGODB_COLLECTION, DB_COLLECTION_NAME);
    map.put(DB_COSMOSDB_CONTAINER, DB_COLLECTION_NAME);

Copy link
Copy Markdown
Contributor

@laurit laurit Oct 29, 2024

Choose a reason for hiding this comment

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

I think we can find ways to resolve the multiple mapping issue. This is just an idea, we can tackle this later if it seems to be worth the effort.

@trask trask merged commit 61c99cf into open-telemetry:main Oct 29, 2024
@trask trask mentioned this pull request Nov 10, 2024
Open
54 tasks
akats7 pushed a commit to akats7/opentelemetry-java-instrumentation that referenced this pull request Nov 21, 2024
@heyams @trask
Implement some of the stable database semantic conventions (open-tele…
d51ca17 
…metry#11575)

Co-authored-by: Trask Stalnaker <[email protected]>
akats7 pushed a commit to akats7/opentelemetry-java-instrumentation that referenced this pull request Nov 21, 2024
@heyams @trask
Implement some of the stable database semantic conventions (open-tele…
946a349 
…metry#11575)

Co-authored-by: Trask Stalnaker <[email protected]>
@lukeina2z lukeina2z mentioned this pull request Sep 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trask trask trask approved these changes

@laurit laurit laurit approved these changes

Assignees

No one assigned

Labels

test native This label can be applied to PRs to trigger them to run native tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@heyams @trask @laurit