Skip to content

OpenTelemetry follow up PR #613

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

Merged
merged 16 commits into from
Sep 20, 2024
Merged

OpenTelemetry follow up PR #613

merged 16 commits into from
Sep 20, 2024

Conversation

joao-r-reis
Copy link
Collaborator

@joao-r-reis joao-r-reis commented Sep 18, 2024

  • Add Distributed Tracing example
  • Add db.query.text support for BoundStatement and BatchStatement
  • Rename builder extension method
  • Add support for speculative executions (mark pending executions as aborted when parent request is completed)

cc @simaoribeiro

@joao-r-reis
Copy link
Collaborator Author

joao-r-reis commented Sep 18, 2024

I just realized that the way we store the node level activity might not work if there are multiple requests going to the same node as part of the same request context. I think in normal circumstances this doesn't happen but it really depends on the load balancing policy (and speculative executions probably need to be enabled as well). I'll probably add a NodeExecutionId to HostTrackingInfo and we can use that id for the key in the context dictionary.

@simaoribeiro
Copy link
Contributor

simaoribeiro commented Sep 18, 2024

Yeah, I think the NodeExecutionId makes sense. I didn't know that use case when used the HostId as part of the key

@joao-r-reis
Copy link
Collaborator Author

Yeah, I think the NodeExecutionId makes sense. I didn't know that use case when used the HostId as part of the key

yeah I just added it now, was a bit of work but it's worth it I think

@joao-r-reis
Copy link
Collaborator Author

joao-r-reis commented Sep 18, 2024

Ok, speculative executions are now supported as well, aborted executions will show up with status "unset", while non aborted executions have the status Ok or Error.

The last thing that I wanted to take care of before releasing this was to add support for Prepared Statements. It would be very useful to have the Prepare round trips on these traces (especially with Mapper and LINQ that automatically prepare stuff internally).

I'm running out of time to get this done though so I'll evaluate tomorrow wether it is feasible to add this before the release.

@simaoribeiro
Copy link
Contributor

simaoribeiro commented Sep 19, 2024

Looks really cool.

Btw, if you pass the number of speculative executions, you can add the attribute db.cassandra.speculative_execution_count. You already did part of the job my marking it unset However, I don't think it's important for now and can be added later.

The prepared statements would be a really useful feature to add but we can also include in the documentation that is not supported for now, so the users are aware.

@joao-r-reis
Copy link
Collaborator Author

Btw, if you pass the number of speculative executions, you can add the attribute db.cassandra.speculative_execution_count. You already did part of the job my marking it unset However, I don't think it's important for now and can be added later.

Unfortunately there's no way to tell whether an execution is speculative or not atm (could just be a retry) so I'd have to implement that on the driver. Something to consider in the future though.

@joao-r-reis
Copy link
Collaborator Author

I'm going to merge this when CI finishes.

@joao-r-reis joao-r-reis merged commit 91643f1 into master Sep 20, 2024
5 checks passed
@joao-r-reis joao-r-reis deleted the otel-example branch September 20, 2024 10:27
@joao-r-reis joao-r-reis added this to the 3.22.0 milestone Sep 27, 2024
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