-
Notifications
You must be signed in to change notification settings - Fork 881
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
CASSJAVA-63: Proposal for Native OpenTelemetry Tracing Support #1994
base: 4.x
Are you sure you want to change the base?
Conversation
proposals/open-telemetry/tracing.md
Outdated
|
||
OpenTelemetry has become the industry standard for telemetry data aggregation, encompassing logs, metrics, and traces. | ||
Tracing, in particular, enables developers to track the full "path" a request takes through the application, providing deep insights into services. | ||
[OpenTelemetry's auto-instrumentation](https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/cassandra/cassandra-4.4/library) of the Apache Cassandra Java Driver (via the Java agent) already supports basic traces, logs, and metrics. However, this proposal to include tracing directly in the native Apache Cassandra Java Driver will eliminate the need for a Java agent and provide more detailed information, including individual Cassandra calls due to retry or speculative execution. |
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 also mention that it will be officially supported and not vulnerable to internal API changes.
|
||
**[2]:** Despite not being required, this implementation sets the `db.operation` attribute even if `db.statement` is included. | ||
|
||
**[3]:** The statement value is the query string and does not include any query values. As an example, having a query that as the string `SELECT * FROM table WHERE x = ?` with `x` parameter of `123`, the attribute value of `db.statement` will be `SELECT * FROM table WHERE x = ?` and not `SELECT * FROM table WHERE x = 123`. |
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.
In extreme cases it may be beneficial to provide also set of bind parameters. For example poorly performing CQL query due to large partition can be discovered when seeing bound parameters. I do not say this feature is strictly needed, but maybe nice to have.
proposals/open-telemetry/tracing.md
Outdated
CqlSession session = CqlSession.builder() | ||
.addContactPoint(new InetSocketAddress("127.0.0.1", 9042)) | ||
.withLocalDatacenter("datacenter1") | ||
.withOpenTelemetry(initOpenTelemetry()) |
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.
If the intention is for driver core module not to declare dependency on OpenTelemetry, this will not be possible. I think we should declare OPTL integration via specific RequestTracker
implementation. When creating a CqlSession
developer will use withRequestTracker()
method to pass new OpenTelemetryRequestTracker(config)
. This way core module will not need to declare API dependency on OpenTelemetry.
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.
Thank you! Good idea!
In that way, will it still support configuration via application.yml
?
proposals/open-telemetry/tracing.md
Outdated
|
||
`OtelRequestTracker` will utilize the above methods. | ||
|
||
`OtelRequestTracker` will initialize OpenTelemetry exporter on `onSessionReady`. |
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 think RequestTracker
is missing close()
method to release resources (thread pools, channels etc.).
proposals/open-telemetry/tracing.md
Outdated
| db.operation.name | The name of the operation being executed. | string | Call | true if `db.statement` is not applicable. [2] | _Session Request_ or _Node Request_ | | ||
| db.query.text | The database statement being executed. | string | Call | false | *database statement in use* [3] | | ||
| server.address | Name of the database host. | string | Connection | true | e.g.: example.com; 10.1.2.80; /tmp/my.sock | | ||
| server.port | Server port number. Used in case the port being used is not the default. | int | Connection | false | e.g.: 9445 | |
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.
Where are we going to pass retry details, speculative execution and consistency level?
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 your review! I updated the proposal. Retries and speculative executions details, e.g. db.query.text
and server.address
will be in the attributes Node Request
type of spans. But we can never know which Node Request
is a retry or a speculative execution. The current RequestTracker
interface does not expose such information.
No description provided.