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

Distributed Tracing/Open Telemetry: Feature requests #4553

Open
Tracked by #4874
sourabh1007 opened this issue Jun 20, 2024 · 6 comments · Fixed by #4765
Open
Tracked by #4874

Distributed Tracing/Open Telemetry: Feature requests #4553

sourabh1007 opened this issue Jun 20, 2024 · 6 comments · Fixed by #4765

Comments

@sourabh1007
Copy link
Contributor

sourabh1007 commented Jun 20, 2024

Query sanitization (#4664)

  1. Parameterized queries shouldn't be sanitized (because the user self-sanitizes through parameters), and collecting parameter values will be an opt in config
  2. For all other queries, text should only be captured if it's sanitized. Literal values should be replaced by ?

Batch ( #4622 )

  1. New attribute to capture batch size
  2. Operation name should be prepended with BATCH for homogenous operations, or be just BATCH for heterogenous operations. If the DB system has a native name that makes sense we can use that (ex. our API names for ReadMany, TransactionalBatch etc.)

Bulk

  1. Operation name should be prepended with Bulk. : Covered as part of Open Telemetry: Fixes Operation name to follow Otel convention #4643
  2. Verify bulk user experience. : Done in Scrum Call

Span name ( #4553 (comment))

  1. Change span name to <operation name> <target> where target is the collection name if available
  2. This removes the <namespace> or account name in our case from the span name

Operation Name db.operation.name (#4643)

Batch/bulk operations:

  • execute_batch
  • execute_bulk
  • batch_< operation name >
  • bulk_< operation name >

Change feed operations:

  • query_change_feed

Conflicts operations:

  • delete_conflict
  • query_conflicts
  • read_all_conflicts
  • read_conflict

Container operations:

  • create_container
  • create_container_if_not_exists
  • delete_container
  • query_containers
  • read_all_containers
  • read_container
  • replace_container

Database operations:

  • create_database
  • create_database_if_not_exists
  • delete_database
  • query_databases
  • read_all_databases

Encryption key operations:

  • create_client_encryption_key
  • query_encryption_keys
  • read_all_encryption_keys
  • read_client_encryption_key
  • replace_client_encryption_key

Item operations:

  • create_item
  • delete_all_items_by_partition_key
  • delete_item
  • patch_item
  • query_items
  • read_all_items
  • read_all_items_of_logical_partition
  • read_many_items
  • read_item
  • replace_item
  • upsert_item

Permission operations:

  • create_permission
  • delete_permission
  • query_permissions
  • read_all_permissions
  • read_permission
  • replace_permission
  • upsert_permission

Stored procedure operations:

  • create_stored_procedure
  • delete_stored_procedure
  • execute_stored_procedure
  • query_stored_procedures
  • read_all_stored_procedures
  • read_stored_procedure
  • replace_stored_procedure

Throughput operations:

  • read_throughput
  • replace_throughput

Trigger operations:

  • create_trigger
  • delete_trigger
  • query_triggers
  • read_all_triggers
  • read_trigger
  • replace_trigger

User operations:

  • create_user
  • delete_user
  • query_users
  • read_all_users
  • read_user
  • replace_user
  • upsert_user

User-defined function operations:

  • create_user_defined_function
  • delete_user_defined_function
  • query_user_defined_functions
  • read_all_user_defined_functions
  • read_user_defined_function

Other changes, as mentioned in #4553 (comment)

(#4765)

db.cosmosdb.operation_type --> db.cosmosdb.operation.type (or we can get rid of this attribute entirely since we have clear operation names now, not sure this still adds any value)
db.cosmosdb.client_id --> db.cosmosdb.client.id (aligns with OTel messaging conventions)
db.cosmosdb.request_content_length --> db.cosmosdb.request.content_length (aligns with OTel HTTP conventions)
db.cosmosdb.request_charge, no change for this one since "request charge" is one thing rather than charge being an attribute of request
db.cosmosdb.consistency_level

Inprogress

a) add db.cosmosdb.regions_contacted
b) change casing for direct and gateway mode
c) add db.cosmosdb.row_count

Versioning

database : Show only stable and new experimental attributes
database/dup : Show both stable and old and new experimental attributes (it means attributes can be duplicated here, if an attribute name was changed from db.cosmos.xx -> db.cosmos.yy, then both the attributes will be emitted)
default: Show only old experimental(or stable) => old will target particular version in case of cosmos db, it is semantic-conventions/docs/database/cosmosdb.md at v1.25.0 · open-telemetry/semantic-conventions (github.com)

Few open requests for reference (might or might not be relevant):

  1. Distributed Tracing: Add db.query.text attribute #4375
  2. Do not treat 404 for a point read as a dependency failure #4095
  3. AI Integration: Get Request size from request diagnostics and optimize logic to get Response Size #3317
@jcocchi
Copy link
Contributor

jcocchi commented Jul 3, 2024

Sanitization reference: sanitization of db.query.text

Batch reference: Guidance throughout multiple areas of OTel database semantic convention common attributes

Span name reference: name

@jcocchi
Copy link
Contributor

jcocchi commented Jul 16, 2024

Adding a few more requirements for attribute changes from the new OTel specification Full list found in the database specification.

Attributes to rename

  • db.cosmosdb.container --> db.collection.name
  • db.name --> db.namespace
  • db.operation --> db.operation.name
  • exception.type --> error.type
    - There are a few other exception attributes, let's confirm if they all need to be changed to prefix with error

New attributes from spec not in our current implementation

  • db.query.text
    - Should only include this if there’s sanitization
  • user_agent.original
    - What happened to this attribute? I don’t see it on the telemetry anymore
- [ ] server.port
- [ ] db.query.parameter.<key>
     - Should only include if there are parameterized queries


Existing attributes to add to spec
Is there a reason we left these off the spec? Once we have a GA release it’s probably good to document them.

  • db.cosmosdb.activity_id
  • db.cosmosdb.machine_id
  • db.cosmosdb.regions_contacted
  • db.cosmosdb.response_content_length_bytes
    - db.cosmosdb.request_content_length is in the spec but not response, is there a reason for that?
    - Should we drop _bytes from the name to match request content length?

CC: @sourabh1007

@sourabh1007
Copy link
Contributor Author

@jcocchi

server.port => is it port number used for RNTBD call? and for http it will be always 443?
user_agent.original => it should be there

this.scope.AddAttribute(OpenTelemetryAttributeKeys.UserAgent, clientContext.UserAgent);

Regarding other attributes, we decided to have these attributes but we left them for otel specs, because these were very specific to cosmos db.

@jcocchi
Copy link
Contributor

jcocchi commented Jul 17, 2024

We can leave off server.port if it isn't useful for us, I just wanted to call out it's in the spec but missing from our implementation.

user_agent.original didn't show up in the list of attributes in my App Insights traces when I ran my sample app yesterday, are you sure it's still populating correctly? Maybe App Insights just doesn't show it in the UI?

Once we GA I think it's okay to add all the attributes to the spec provided we don't plan to change them.

@sourabh1007
Copy link
Contributor Author

Span name is already <operationname><space><collectionname>

image

Code ref.

@sourabh1007
Copy link
Contributor Author

What do you think about changing, db.cosmosdb.operation_type to db.operation.type since we have db.operation.name and db.operation.batch.size?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants