-
Notifications
You must be signed in to change notification settings - Fork 494
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
AI Integration: OTel CosmosDB Attributes to Collect #3058
Comments
Few comments:
|
Connection Mode - Db URL is actually just an Account Name, So you can't actually identify if it is direct or gateway mode by this url. Added list of Default Information |
Thank you for summarizing it! I have a couple of comments:
|
I think we can make both the changes so replacing |
I disagree on replacing db.url with net.peer.name and net.peer.port - the url contains also critical information like partitionId and replicaId = the dns-hostname/ip + port is only pointing to the transport endpoint which can be considered a load pbalancer in front of the pods used for the replica |
@FabianMeiswinkel you need to join Microsoft org to see this repo: https://repos.opensource.microsoft.com/orgs/microsoft
The critical information should go to dedicated attributes - let's list what's critical. We could also keep The reason is that APM tools don't know what If we put critical info into dedicated attributes instead, we'd allow users to query by partitionId or replicaId. As long as those are part of url, we won't have this capability with an arbitrary tracing tool. |
@FabianMeiswinkel This |
@lmolkova do you mean |
why so? I assume it could be useful to write queries per container or per db and also it contradicts OTel conventions - check out what Mongo does there. Any reason why we want to keep them in a single attribute? |
Let me summarize the changes here:
|
Thank you @sourabh1007, it looks good! I few minor things
|
|
Thanks @sourabh1007
Information that appears on |
Can we use a full hostname like Also, is there ever a case where non-default port is used on client side? If so, we should also add |
@lmolkova we intentionally used an identifier which can't be resolved into a real hostname/port. The traces above are logical requests (they might end-up hitting x endpoints). net.peer.name and net.peer.port form the name (and semantic usage of how other databases are using it) indicates that this would be a physical network endpoint. You mentioned that an identifier that can be used to identify a resource for Cosmos is required. So we used an identifier that identifies the account - but to avoid confusion used one that clearly is not a physical networking endpoint. Reason why this seems important is that the vast majority of physical communication is not going to this DNS hostname + port (but the actual backend replica/shards) |
@FabianMeiswinkel, thanks for the context and it makes total sense. My only worry is that |
I am totally fine with adding a prefix/suffix for cloud - just want to avoid that anyone would think this is a real network endpoint |
Been playing with attributes, a few more points:
|
1. |
|
Azure Monitor supports reading Azure-service specific identifiers (on Functions, AppServices, Azure VMs) and falls back to machine name, which in container environments, I believe, is container id. There is no need to replicate this complicated logic. |
Agreed as long as there is really an easy way to get containerId/nodeId from what is getting logged. I can just tell form past experience that neither on Azure Functions (automatic logging) nor App Services (manual logging) there has been an easy way for customers to find this info when they logged to AppInsights. |
Closing as it is already implemented. |
Status: Experimental
Target SDK: .Net SDK
Operations To Cover
Database / Container Operations : Cosmos.CreateDatabaseAsync / Cosmos.CreateContainerAsync / Cosmos.DeleteStreamAsync
Point Operations : Cosmos.CreateItemAsync / Cosmos.ReadItemAsync / Cosmos.UpsertItemAsync / Cosmos.ReplaceItemAsync / Cosmos.PatchItemAsync / Cosmos.DeleteItemAsync
Stream Operations : Cosmos.CreateItemStreamAsync / Cosmos.UpsertItemStreamAsync / Cosmos.ReadItemStreamAsync / Cosmos.ReplaceItemStreamAsync / Cosmos.PatchItemStreamAsync / Cosmos.DeleteItemStreamAsync
Batch Operations : TBD
Bulk Operation : TBD
Query Operations : Cosmos.Typed FeedIterator ReadNextAsync (for each page)
Attributes
AI Default Attributes:
Other default attributes are there e.g device details
Proposal For .Net Cosmos DB SDK
Custom Attributes:
Azure Specific Attributes
Common Database Attributes
TypeNameCosmos DB Specific
Account Level Information:
Request Level Information:
Open Question: What if this string is out of the limit size? will appinsight will break it and divide into different attributes?Generating it as eventOpen Telemetry Standard for any Exception
java.net.ConnectException; ``OSError
Division by zero; Can't convert 'int' object to str implicitly
Exception in thread "main" java.lang.RuntimeException: Test exception\n at com.example.GenerateTrace.methodB(GenerateTrace.java:13)\n at com.example.GenerateTrace.methodA(GenerateTrace.java:9)\n at com.example.GenerateTrace.main(GenerateTrace.java:5)
Following Open Telemetry Conventions (Note: Status of below conventions is Experimental):
1. https://github.com/Azure/azure-sdk/blob/main/docs/tracing/distributed-tracing-conventions.yml2. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md
3. https://opentelemetry.io/docs/reference/specification/common/attribute-naming/
The text was updated successfully, but these errors were encountered: