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

Add tracing instrumentation #80

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ogxd
Copy link

@ogxd ogxd commented Mar 29, 2023

No description provided.

@ogxd ogxd mentioned this pull request Mar 29, 2023
@shannonklaus shannonklaus changed the base branch from master to stage March 30, 2023 16:03
@shannonklaus shannonklaus changed the base branch from stage to master March 30, 2023 16:41

internal const string SERVICE_NAME = "Aerospike";

internal static readonly ActivitySource Source = new ActivitySource(SOURCE_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add the version of the assembly in the ActivitySource ctor

// https://github.com/open-telemetry/opentelemetry-specification/blob/6ce62202e5407518e19c56c445c13682ef51a51d/specification/trace/semantic_conventions/span-general.md#general-remote-service-attributes
activity.SetTag("peer.service", Tracing.SERVICE_NAME);
// https://github.com/open-telemetry/opentelemetry-specification/blob/6ce62202e5407518e19c56c445c13682ef51a51d/specification/trace/semantic_conventions/database.md#connection-level-attributes
activity.SetTag("db.system", Tracing.SERVICE_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the spec, db.system should be lowercased (Aerospike -> aerospike). I've tried adding it to the spec but kind of got ignored open-telemetry/opentelemetry-specification#2090

activity?.Dispose();
activity = Tracing.Source.StartActivity(GetType().Name, ActivityKind.Client);
if (activity != null)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be great to have the following tags:

  • db.users
  • network.peer.address
  • network.peer.port
  • db.name
  • db.operation.
  • db.statement

if (activity != null)
{
// https://github.com/open-telemetry/opentelemetry-specification/blob/6ce62202e5407518e19c56c445c13682ef51a51d/specification/trace/semantic_conventions/span-general.md#general-remote-service-attributes
activity.SetTag("peer.service", Tracing.SERVICE_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

The service name is one of the most important information. When the trace is displayed on a UI (Jaeger, Tempo), it will be the most visible info. I would avoid hardcoding it but either use the cluster name or a new user-provided string.


// If we're retrying, stop the previous Activity before restarting a new one.
activity?.Dispose();
activity = Tracing.Source.StartActivity(GetType().Name, ActivityKind.Client);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

According to the spec, we should try to make the span name look like <db.operation> <db.name>.<db.sql.table>. Also this string should probably be cached as we don't want such allocation in the very hot path.

// If we're retrying, stop the previous Activity before restarting a new one.
activity?.Dispose();
activity = Tracing.Source.StartActivity(GetType().Name, ActivityKind.Client);
if (activity != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

activity.IsAllDataRequested should also be checked before setting tags.

Comment on lines +222 to +224
activity.SetTag("node.name", node.Name);
activity.SetTag("node.address", node.NodeAddress.ToString());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Those don't seem to be standard tags, I would use network.peer* as mentioned below.

activity.SetTag("peer.service", Tracing.SERVICE_NAME);
// https://github.com/open-telemetry/opentelemetry-specification/blob/6ce62202e5407518e19c56c445c13682ef51a51d/specification/trace/semantic_conventions/database.md#connection-level-attributes
activity.SetTag("db.system", Tracing.SERVICE_NAME);
activity.SetTag("iteration", iteration);
Copy link
Contributor

Choose a reason for hiding this comment

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

There doesn't seem to be a standard for retries tags. Though there is http.request.resend_count so maybe we could name it db.resend_count?

{
if (activity != null && exception != null)
{
activity.SetStatus(ActivityStatusCode.Error, exception.ToString());
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the status description should contain the full exception. There are exception tags made for this https://opentelemetry.io/docs/specs/semconv/attributes-registry/exception. See how they do it in OpenTelemetry.Api: https://github.com/open-telemetry/opentelemetry-dotnet/blob/14ee5c456ee91ffc11e29b35333834b2e04f53d3/src/OpenTelemetry.Api/Trace/TelemetrySpan.cs#L266.

That would allow a UI to format the exception if the rights tags are present.

@shannonklaus
Copy link
Collaborator

@ogxd @verdie-g Sorry for the delay. I would like to proceed with testing this and possibly getting it merged for the next release, pending the testing. Can you two resolve the comments so I can proceed?

@verdie-g
Copy link
Contributor

I could be interested to recreate the PR if @ogxd is doesn't have the time to work on it.

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.

3 participants