-
Notifications
You must be signed in to change notification settings - Fork 829
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
Implement stable database semantic conventions #11575
base: main
Are you sure you want to change the base?
Implement stable database semantic conventions #11575
Conversation
.../opentelemetry/javaagent/instrumentation/lettuce/v5_0/LettuceConnectAttributesExtractor.java
Outdated
Show resolved
Hide resolved
…base-stable-semconv
...main/java/io/opentelemetry/javaagent/instrumentation/cassandra/v3_0/CassandraSingletons.java
Outdated
Show resolved
Hide resolved
...pentelemetry/instrumentation/api/incubator/semconv/db/DbClientCommonAttributesExtractor.java
Outdated
Show resolved
Hide resolved
…base-stable-semconv
…base-stable-semconv
…base-stable-semconv
I will add 'error.type' and the opt-in |
...java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientAttributesGetter.java
Outdated
Show resolved
Hide resolved
...o/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientCommonAttributesGetter.java
Outdated
Show resolved
Hide resolved
...o/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientCommonAttributesGetter.java
Show resolved
Hide resolved
SemconvStabilityUtil.getAttributeKey(DbIncubatingAttributes.DB_STATEMENT), | ||
"SELECT *"), | ||
entry( | ||
SemconvStabilityUtil.getAttributeKey(DbIncubatingAttributes.DB_OPERATION), | ||
"SELECT")); |
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'd suggest changing these two lines to reference the stable names directly
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.
but we test both non-stable and stable. are you suggest to change gradle config to test stable only?
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 else
condition is only executed when not emitting old db semconv
SemconvStabilityUtil.getAttributeKey(DbIncubatingAttributes.DB_STATEMENT), | ||
"SELECT * FROM table"), | ||
entry( | ||
SemconvStabilityUtil.getAttributeKey(DbIncubatingAttributes.DB_OPERATION), | ||
"SELECT"), | ||
entry( | ||
SemconvStabilityUtil.getAttributeKey(DbIncubatingAttributes.DB_CASSANDRA_TABLE), |
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.
same here
...tation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/SemconvStability.java
Outdated
Show resolved
Hide resolved
/** | ||
* @deprecated Use {@link #getDbQueryText(REQUEST)} instead. | ||
*/ | ||
@Deprecated | ||
@Nullable | ||
String getStatement(REQUEST request); |
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.
since this was just a rename, I think could add default implementation that delegates to getDbQueryText
, then wouldn't need to override this everywhere
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's better to use @deprecate so that when old semconv is removed, we can use it to clean it up.
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.
you can still keep @Deprecated
on the default implementation
/** | ||
* @deprecated Use {@link #getOperationName(REQUEST)} instead. | ||
*/ | ||
@Deprecated | ||
@Nullable | ||
String getOperation(REQUEST request); |
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.
similar here could add default method calling getDbOperationName
if (SemconvStability.emitOldDatabaseSemconv()) { | ||
request.put("db.statement", "SELECT * FROM table"); | ||
} else { | ||
request.put("db.query.text", "SELECT * FROM table"); | ||
} |
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.
do we test also with database/dup
? if so should this instead be:
if (SemconvStability.emitOldDatabaseSemconv()) { | |
request.put("db.statement", "SELECT * FROM table"); | |
} else { | |
request.put("db.query.text", "SELECT * FROM table"); | |
} | |
if (SemconvStability.emitOldDatabaseSemconv()) { | |
request.put("db.statement", "SELECT * FROM table"); | |
} | |
if (SemconvStability.emitNewDatabaseSemconv()) { | |
request.put("db.query.text", "SELECT * FROM table"); | |
} |
Protyping open-telemetry/semantic-conventions#1090