-
Notifications
You must be signed in to change notification settings - Fork 101
feat: add cloud.region, request_tag and transaction_tag in span attributes #1449
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
Conversation
b5ca757 to
af65849
Compare
f5bed57 to
1bd0a2c
Compare
15f395e to
6059b9b
Compare
| "enable_end_to_end_tracing", enable_end_to_end_tracing | ||
| ) | ||
| db_name = observability_options.get("db_name", db_name) | ||
| cloud_region = observability_options.get("cloud_region", cloud_region) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the use of this ?
IMU, observability_options is something which customers can set and does not have cloud_region property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line is not required as _get_cloud_region is overriding the value in next line. Removed
| if "request_options" in attributes: | ||
| request_options = attributes.pop("request_options") | ||
| if request_options and request_options.request_tag: | ||
| attributes["spanner.request_tag"] = request_options.request_tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use attribute name as request.tag and transaction.tag to be consistent with other languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need the same for transaction.tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored to request.tag. transaction tag is already transaction.tag so it shouldn't need any change right?
| # Overwrite the requests timeout for the detector. | ||
| # This is necessary as the client will wait the full timeout if the | ||
| # code is not run in a GCP environment, with the location endpoints available. | ||
| gcp_resource_detector._TIMEOUT_SEC = 0.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check if we need this timeout in the new code after refactoring. Currently get region does not have any timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes got missed while moving the logic to helper. Added back
6059b9b to
e0a2fb9
Compare
e0a2fb9 to
f3bf074
Compare
9d2d9e0 to
2ff6bf1
Compare
This change enhances observability by introducing these new features:
Cloud Region Attribute: The
cloud.regionattribute is now added to all OpenTelemetry spans generated by the Spanner client. This provides better geographical context for traces, aiding in performance analysis and debugging across different regions.Transaction Tag: The
transaction_tagset on aTransactionobject is now correctly propagated and included in theCommitrequest. This allows for better end-to-end traceability of transactions.Request Tag: This introduces support for
request_tagon individual Spanner operations likeread,execute_sql, andexecute_update. When arequest_tagis provided in therequest_options, it is now added as aspanner.request_tagattribute to the corresponding OpenTelemetry span. This allows for more granular tracing and debugging of specific requests within a transaction or a snapshot.