Skip to content

Implement command tracing #165

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

verdie-g
Copy link
Contributor

@verdie-g verdie-g commented Jun 12, 2025

Closes #79, supersedes #80.

Following the OTEL semconv https://opentelemetry.io/docs/specs/semconv/database/database-spans.

@shannonklaus could help me with that:

  1. The span (Activity) needs to be disposed when the command ends, even if it failed for any reason. Where is the most reliable place I could do that? I noticed that if an exception is thrown if a command fails, AsyncCommand.Finish is not called.
  2. Ideally we should set the span status to OK or ERROR and the tag db.response.status_code. Though, it seems like we only have this information in the individual ParseResult methods. I'm wondering if there is a single place where we could set the status.
  3. In the tests, I would like to add a case for timeout, retry, command specific error (e.g. hot key). Any idea how to simulate that?
  4. For the command names I used the ones found there https://aerospike.com/docs/develop/learn/single. Is there an official names for each command?

verdie-g and others added 4 commits June 12, 2025 09:52
Could not find a way to implement it. Tested manually the retry, it
seems to work fine.
@verdie-g verdie-g marked this pull request as ready for review June 24, 2025 03:42
@shannonklaus
Copy link
Collaborator

I expect to have time to look at this in the next two weeks or so. I don't currently know of an easy place to Dispose or set the status so I'll need to take some time to see what I can do. I appreciate the idea for the types of tests you suggested but since our tests aren't true unit tests (no server mock class, and developing one is not a trivial task) I stimulate situations like this manually.

@shannonklaus
Copy link
Collaborator

shannonklaus commented Jul 10, 2025

A few questions for you:

  1. Disposing of the span is a bit more complex than just one place. As you noted, Finish is only called on command success. A command has three fates it can take: 1) success, which is covered in Finish; 2) failure and retry, which is covered in AsyncCommand.Retry; 3) failure and end, which I think you have covered. So we are currently missing retry. Edit: Also missing in OnSocketError()
  2. For the db.response.status_code are you wanting the result code returned from the server? If so we have this info in ParseHeader() for async commands inheriting from AsyncSingleCommand (everything except batch, scan, and query). ResultCode is accessible for multi commands in ParseGroup() from AsyncMultiCommand, though individual responses may have different result codes as it is a per-record response result code. As you noted whether a non-zero result code is classified as an error/failure is in the individual ParseResult methods, so I wanted to clarify what you wanted here.

@verdie-g
Copy link
Contributor Author

So we are currently missing retry.

I believe the retry is covered by the EndSpan in OnError.

Also missing in OnSocketError()

Added a EndSpan there. Some paths could call EndSpan twice now but I handled that case.

For the db.response.status_code are you wanting the result code returned from the server?

Yes

If so we have this info in ParseHeader() for async commands inheriting from AsyncSingleCommand

True but there is no central place to get that value right ?

@shannonklaus
Copy link
Collaborator

I'm not really sure what you mean by central place. If you mean "is there one place to set the status_code?", the answer is no. As I stated, ParseHeader covers the commands with a single response. The result code for commands with multiple responses is more complex since it's on a per record basis, so I think you would need something to keep track of the overall status of the command. I'm not familiar with tracing so I'm not sure what you need for this.
There is AsyncCommand OnSuccess() and OnFailure() which can indicate overall command success or failure but currently we have already disposed of the span when these are called and these don't cover the retry case.

@verdie-g
Copy link
Contributor Author

In case of retry, the new command was started before the span of the previous command was disposed. It should be better now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tracing
2 participants