Skip to content

Adds to REDIS instrumentation message value as tag #1972

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Adds to REDIS instrumentation message value as tag #1972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac92ee3
Adds to REDIS instrumentation message value as tag
yartat Apr 11, 2021
d08ca4f
Refactoring code
yartat Apr 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion ...nstrumentation.StackExchangeRedis/Implementation/RedisProfilerEntryToActivityConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,17 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.Net;
using System.Reflection;
using OpenTelemetry.Trace;
using StackExchange.Redis.Profiling;

namespace OpenTelemetry.Instrumentation.StackExchangeRedis.Implementation
{
internal static class RedisProfilerEntryToActivityConverter
{
private const string MessageFieldValue = "Message";
private static FieldInfo messageInfo;

public static Activity ProfilerCommandToActivity(Activity parentActivity, IProfiledCommand command)
{
var name = command.Command; // Example: SET;
Expand All @@ -42,7 +46,7 @@ public static Activity ProfilerCommandToActivity(Activity parentActivity, IProfi
return null;
}

if (activity.IsAllDataRequested == true)
if (activity.IsAllDataRequested)
{
// see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md

Expand All @@ -62,6 +66,13 @@ public static Activity ProfilerCommandToActivity(Activity parentActivity, IProfi
activity.SetTag(SemanticConventions.AttributeDbSystem, "redis");
activity.SetTag(StackExchangeRedisCallsInstrumentation.RedisFlagsKeyName, command.Flags.ToString());

var message = GetMessageValue(command);
if (!string.IsNullOrEmpty(message))
{
// Example: "db.operation": [0]:GET key1
activity.SetTag(SemanticConventions.AttributeDbOperation, message);
Copy link
Member

@cijothomas cijothomas Apr 13, 2021

Choose a reason for hiding this comment

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

From semantic conventions doc, db.operation is required only if db.statement is not applicable. (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md#call-level-attributes)
Since we already populate the db.statement, we need to have good reason for populating db.operation considering its not free to obtain the value for it.

Copy link
Author

@yartat yartat Apr 19, 2021

Choose a reason for hiding this comment

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

Thanks a lot for review.
I've tested and had have next results

Read data from REDIS
db.redis.database_index=0
db.redis.flags=1024
db.statement=GET
peer.service=xx.xx.xx.xx:6379
net.peer.ip=xx.xx.xx.xx
db.operation=[0]:GET sso:templates:String:USERRESET (RedisValueProcessor)
Delete key in REDIS
db.redis.database_index=0
db.redis.flags=1028
db.statement=UNLINK
peer.service=xx.xx.xx.xx:6379
net.peer.ip=xx.xx.xx.xx
db.operation=[0]:UNLINK sso:templates:String:USERRESET (DemandZeroOrOneProcessor)
Write JSON to REDIS
db.redis.database_index=0
db.redis.flags=1028
db.statement=SETEX
peer.service=xx.xx.xx.xx:6379
net.peer.ip=xx.xx.xx.xx
db.operation=[0]:SETEX sso:templates:String:USERRESET (BooleanProcessor)

So, db.operation contains REDIS operation, db number and key. Semantic conventions doc contains next text

When setting this to an SQL keyword, it is not recommended to attempt any client-side parsing of db.statement just to get this property, but it should be set if the operation name is provided by the library being instrumented.

For Redis, the value provided for db.statement SHOULD correspond to the syntax of the Redis CLI. If, for example, the HMSET command is invoked, "HMSET myhash field1 'Hello' field2 'World'" would be a suitable value for db.statement.

db.statement does not contain key and value. db.operation contains key but does not contain value. It is much more information to analyze!
How would you like an idea if I add parsing db.operation to get Redis CLI-like to match the semantic conventions doc?

}

if (command.Command != null)
{
// Example: "db.statement": SET;
Expand Down Expand Up @@ -115,5 +126,16 @@ public static void DrainSession(Activity parentActivity, IEnumerable<IProfiledCo
ProfilerCommandToActivity(parentActivity, command);
}
}

private static string GetMessageValue(IProfiledCommand command)
{
if (messageInfo == null)
{
messageInfo = command.GetType().GetField(MessageFieldValue, BindingFlags.NonPublic | BindingFlags.Instance);
}

var message = messageInfo?.GetValue(command);
return message?.ToString();
}
}
}
30 changes: 30 additions & 0 deletions test/OpenTelemetry.Instrumentation.StackExchangeRedis.Tests/Implementation/Message.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// <copyright file="Message.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

namespace OpenTelemetry.Instrumentation.StackExchangeRedis.Implementation
{
internal sealed class Message
{
private readonly string messageValue;

public Message(string value)
{
this.messageValue = value;
}

public override string ToString() => this.messageValue;
}
}
74 changes: 74 additions & 0 deletions ...nTelemetry.Instrumentation.StackExchangeRedis.Tests/Implementation/ProfiledCommandStub.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// <copyright file="ProfiledCommandStub.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using System.Net;
using StackExchange.Redis;
using StackExchange.Redis.Profiling;

namespace OpenTelemetry.Instrumentation.StackExchangeRedis.Implementation
{
internal sealed class ProfiledCommandStub : IProfiledCommand
{
internal Message Message;

public ProfiledCommandStub(
int db,
string command,
string message)
{
this.Db = db;
this.Command = command;
this.Message = new Message(message);
}

/// <inheritdoc/>
public EndPoint EndPoint => null;

/// <inheritdoc/>
public int Db { get; }

/// <inheritdoc/>
public string Command { get; }

/// <inheritdoc/>
public CommandFlags Flags => CommandFlags.None;

/// <inheritdoc/>
public DateTime CommandCreated => DateTime.UtcNow;

/// <inheritdoc/>
public TimeSpan CreationToEnqueued => TimeSpan.Zero;

/// <inheritdoc/>
public TimeSpan EnqueuedToSending => TimeSpan.Zero;

/// <inheritdoc/>
public TimeSpan SentToResponse => TimeSpan.Zero;

/// <inheritdoc/>
public TimeSpan ResponseToCompletion => TimeSpan.Zero;

/// <inheritdoc/>
public TimeSpan ElapsedTime => TimeSpan.Zero;

/// <inheritdoc/>
public IProfiledCommand RetransmissionOf => null;

/// <inheritdoc/>
public RetransmissionReasonType? RetransmissionReason => null;
}
}
16 changes: 16 additions & 0 deletions ...ion.StackExchangeRedis.Tests/Implementation/RedisProfilerEntryToActivityConverterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,22 @@ public void ProfilerCommandToActivity_UsesDnsEndPointAsEndPoint()
Assert.Equal(dnsEndPoint.Port, result.GetTagValue(SemanticConventions.AttributeNetPeerPort));
}

[Fact]
public void ProfilerCommandToActivity_UsesMessageForDbOperation()
{
const int db = 0;
const string command = "GET";
const string message = "[0]: GET key";

var activity = new Activity("redis-profiler");
var profiledCommand = new ProfiledCommandStub(db, command, message);

var result = RedisProfilerEntryToActivityConverter.ProfilerCommandToActivity(activity, profiledCommand);

Assert.NotNull(result.GetTagValue(SemanticConventions.AttributeDbOperation));
Assert.Equal(message, result.GetTagValue(SemanticConventions.AttributeDbOperation));
}

#if !NET461
[Fact]
public void ProfilerCommandToActivity_UsesOtherEndPointAsEndPoint()
Expand Down