Skip to content
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

Add tracing to the Cosmos Client #23185

Closed
kjg opened this issue Jul 12, 2024 · 11 comments · Fixed by #23268
Closed

Add tracing to the Cosmos Client #23185

kjg opened this issue Jul 12, 2024 · 11 comments · Fixed by #23268
Labels
Client This issue points to a problem in the data-plane of the library. Cosmos customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that Service Attention Workflow: This issue is responsible by Azure service team.

Comments

@kjg
Copy link
Contributor

kjg commented Jul 12, 2024

Feature Request

I see that many of the packages such as aztables and azidentity have build in tracing support, but it appears that azcosmos does not yet have this.

Is this on the roadmap at all or would you be open to accepting a contribution adding this to the cosmos implementation?

@github-actions github-actions bot added Client This issue points to a problem in the data-plane of the library. Cosmos customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that Service Attention Workflow: This issue is responsible by Azure service team. labels Jul 12, 2024
Copy link

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @MehaKaushik @Pilchie @wmengmsft.

@Pilchie
Copy link
Member

Pilchie commented Jul 15, 2024

Also tagging @simorenoh and @ealsur.

@jhendrixMSFT
Copy link
Member

This should be pretty straightforward, just need to add calls to runtime.StartSpan in the APIs. It should look like this. Just be sure that err doesn't get shadowed so that the correct value is captured.

@kjg
Copy link
Contributor Author

kjg commented Jul 16, 2024

@jhendrixMSFT it looks like the Tracer is added to all azcore Client based clients, but cosmos seems to use Pipelines directly without an azcore Client. Would it make sense to just add this little bit from azcore's NewClient into cosmos's NewClient functions?

Also, would you like for me to open a PR for these changes, or do you foresee this being added from your side? Thanks!!

@jhendrixMSFT
Copy link
Member

Good point. We will also need to update the azcosmos.Client to use the azcore.Client internally so this is properly wired up.

@jhendrixMSFT
Copy link
Member

@ealsur I took a stab at updating the cosmos client to use an *azcore.Client internally. Looks like a lot of tests will need to be updated though. Not sure how you want to proceed here but below is my conversion so you can get an idea.

jhendrixMSFT@859e79f

@Pilchie
Copy link
Member

Pilchie commented Jul 17, 2024

Thanks @jhendrixMSFT. Just wanted to point out that it might be a couple of weeks before we're able to pick this up.

@ealsur
Copy link
Member

ealsur commented Jul 30, 2024

@jhendrixMSFT Is the recommendation that all clients inherit from azcore.Client? Which are the benefits? Otherwise we could simply create a Tracer of our own?

Creating the Tracer would not be enough though right? We would need to wire the Tracer on all operations?

@jhendrixMSFT
Copy link
Member

Not inherit but embed. This way, you get all the wiring and any other future changes for free. If not, you'd want to reimplement the body of NewClient with the same content.

@ealsur
Copy link
Member

ealsur commented Jul 31, 2024

@jhendrixMSFT Let's say we embed it, do we need to implement calls to the tracer anyway around the user methods? Just trying to measure the effort required.

@jhendrixMSFT
Copy link
Member

Yeah, each method will require adding a call to runtime.StartSpan.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. Cosmos customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that Service Attention Workflow: This issue is responsible by Azure service team.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants