Skip to content

CASSJAVA-63: Proposal for Native OpenTelemetry Tracing Support#1994

Open
SiyaoIsHiding wants to merge 6 commits intoapache:4.xfrom
SiyaoIsHiding:otel-proposal
Open

CASSJAVA-63: Proposal for Native OpenTelemetry Tracing Support#1994
SiyaoIsHiding wants to merge 6 commits intoapache:4.xfrom
SiyaoIsHiding:otel-proposal

Conversation

@SiyaoIsHiding
Copy link
Contributor

No description provided.

Copy link
Member

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`.
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.).

Copy link
Member

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?

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments