-
Notifications
You must be signed in to change notification settings - Fork 839
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
feat(cassandra4): more attributes #1314
feat(cassandra4): more attributes #1314
Conversation
hey @FrankSpitulski! we had some discussion recently in #602 and open-telemetry/opentelemetry-specification#968, and want to spec even instrumentation-specific attributes can you submit a PR to the specification repo first? this is probably where I would start: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/database.md#call-level-attributes-for-specific-technologies |
instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/MoreAttributes.java
Outdated
Show resolved
Hide resolved
@FrankSpitulski Are you interested in open a PR in spec repo adding those semantic attributes? |
Sure, they tagged the issue I made release:after-ga though so I didn't think it was going to go in. |
hey @FrankSpitulski, i've asked for clarification open-telemetry/opentelemetry-specification#1058 (comment) |
39b9bdd
to
021c905
Compare
7280c2a
to
3465108
Compare
@trask should be good now |
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 - small nits
...io/opentelemetry/javaagent/instrumentation/cassandra/v4_0/CassandraDatabaseClientTracer.java
Outdated
Show resolved
Hide resolved
...io/opentelemetry/javaagent/instrumentation/cassandra/v4_0/CassandraDatabaseClientTracer.java
Outdated
Show resolved
Hide resolved
e50164f
to
2e634fd
Compare
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!
2e634fd
to
f8907df
Compare
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 getting this into the specs!!
...io/opentelemetry/javaagent/instrumentation/cassandra/v4_0/CassandraDatabaseClientTracer.java
Outdated
Show resolved
Hide resolved
...io/opentelemetry/javaagent/instrumentation/cassandra/v4_0/CassandraDatabaseClientTracer.java
Outdated
Show resolved
Hide resolved
instrumentation/cassandra/cassandra-4.0/javaagent/src/test/groovy/CassandraClientTest.groovy
Outdated
Show resolved
Hide resolved
...a/io/opentelemetry/javaagent/instrumentation/cassandra/v4_0/CassandraTableNameExtractor.java
Outdated
Show resolved
Hide resolved
e0ccb55
to
ae24113
Compare
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.
@FrankSpitulski thanks for your patience with us, and great work again driving this through the specification process!
@FrankSpitulski can you please rebase this PR? Then it can finally be merged. Sorry for such slow progress :( |
fixes open-telemetry#1298 Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
…ovy/CassandraClientTest.groovy Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
901e4fb
to
f640dd1
Compare
4be78fa
to
1dba7dc
Compare
@iNikem I have rebased and updated the code to use the new SqlStatementSanitizer. As @mateuszrzeszutek mentioned it can no longer handle CREATE statements. It also no longer splits out the table name from the keyspace when the statement has the form |
Let's go ahead and get this in - @mateuszrzeszutek do you mind following up with the SQL issues? |
@FrankSpitulski Thanks a lot for this contribution and bearing with the long review process! 🙏 |
I'll create an issue for that 👍
Actually, this is intentional: spec says that
so whatever sanitizer extracts as "table" may contain the schema name. |
🎉 thank you @FrankSpitulski |
fixes #1298