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

feat(observability): add tracing spans to Session #2121

Closed
wants to merge 2 commits into from

Conversation

odeke-em
Copy link
Contributor

This change adds tracing spans to Session for methods:

  • create
  • getMetadata
  • keepAlive

and corresponding tests.

Updates #2079
Built from PR #2087
Updates #2114

@odeke-em odeke-em requested review from a team as code owners September 21, 2024 01:49
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: spanner Issues related to the googleapis/nodejs-spanner API. labels Sep 21, 2024
@odeke-em odeke-em force-pushed the trace-Session branch 5 times, most recently from 909108d to d670cd6 Compare September 23, 2024 07:45
This change adds tracing spans to Session for methods:
* create
* getMetadata
* keepAlive

and corresponding tests.

Updates googleapis#2079
Built from PR googleapis#2087
Updates googleapis#2114
Copy link
Contributor

@surbhigarg92 surbhigarg92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we dont need this PR. All the important spans are already there for sessions.

Lete keep this on hold and move to other PRs.

@@ -431,6 +454,10 @@ export class Session extends common.GrpcServiceObject {
optionsOrCallback?: CallOptions | KeepAliveCallback,
cb?: KeepAliveCallback
): void | Promise<KeepAliveResponse> {
// NOTE: Please do not trace Ping as it gets quite spammy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this comment, this comment was already added in the caller of this method.

resp.databaseRole = resp.creatorRole;
delete resp.creatorRole;
this.metadata = resp;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove for this method. We dont need it for now. Can add later if we need

return;
}
const q = {opts: database.observabilityConfig};
return startTrace('Session.create', q, span => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we dont need this as well, as we dont use single session creation , instead we use batch create session

@odeke-em
Copy link
Contributor Author

Alright, thanks @surbhigarg92! Sure we can put it on ice and move onto others!

@odeke-em
Copy link
Contributor Author

odeke-em commented Oct 8, 2024

Deemed as unnecessary!

@odeke-em odeke-em closed this Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/nodejs-spanner API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants