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.

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

[Instrumentation.EntityFrameworkCore] Fix db.statement tag for Devart Oracle #2466

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

[Instrumentation.EntityFrameworkCore] Fix db.statement tag for Devart Oracle #2466

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7b66dc5
Fix db.statement tag for Devart Oracle
vladajankovic Jan 15, 2025
f455d86
Update CHANGELOG.md
vladajankovic Jan 15, 2025
6e52578
Fix error
vladajankovic Jan 15, 2025
a8a7651
Merge branch 'main' into fix_db_statement_tag_for_devart_oracle
Kielek Jan 16, 2025
b2ea7d8
Update CHANGELOG.md
vladajankovic Jan 16, 2025
73b0921
Fix lint
vladajankovic Jan 16, 2025
3369e1b
Update code to support both emitting old and new attributes
vladajankovic Jan 16, 2025
175c984
Merge branch 'main' into fix_db_statement_tag_for_devart_oracle
Kielek Jan 30, 2025
e7adb6e
Merge branch 'main' into fix_db_statement_tag_for_devart_oracle
Kielek Feb 17, 2025
f0c4f46
Merge branch 'main' into fix_db_statement_tag_for_devart_oracle
Kielek Feb 18, 2025
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
5 changes: 5 additions & 0 deletions src/OpenTelemetry.Instrumentation.EntityFrameworkCore/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@
* Attribute `db.system` reports `oracle` when
`Devart.Data.Oracle.Entity.EFCore` is used a provider.
([#2465](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2465))

* Added code to fix exception thrown when Devart Oracle is used
(`this.commandTypeFetcher.Fetch(command)`). If Devart Oracle is used,
the SQL statement will be extracted from the payload.
([#2466](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2466))

## 1.10.0-beta.1

Expand Down
15 changes: 14 additions & 1 deletion ...y.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,20 @@ public override void OnEventWritten(string name, object? payload)
return;
}

if (this.commandTypeFetcher.Fetch(command) is CommandType commandType)
if (command.GetType().FullName?.Contains("Devart.Data.Oracle") == true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider code below. It is doing couple of things

  1. It is always trying to fetch method in the standard way (probably most common)
  2. If it fails it goes to fallback mechanism for Devart.
  3. Changes some Linq comparison to standard way.
  4. Optimize payloadString.Contains( usages in new code, it is better to check bools first (cheaper).
From a06207733b5917f57328bd281d6d586948719e79 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Piotr=20Kie=C5=82kowicz?= <[email protected]>
Date: Wed, 22 Jan 2025 09:31:15 +0100
Subject: [PATCH] proposal

---
 .../EntityFrameworkDiagnosticListener.cs      | 116 ++++++++----------
 1 file changed, 53 insertions(+), 63 deletions(-)

diff --git a/src/OpenTelemetry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs
index 26eb87b9..0cdb07fe 100644
--- a/src/OpenTelemetry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs
+++ b/src/OpenTelemetry.Instrumentation.EntityFrameworkCore/Implementation/EntityFrameworkDiagnosticListener.cs
@@ -200,90 +200,80 @@ internal sealed class EntityFrameworkDiagnosticListener : ListenerHandler
                             return;
                         }
 
-                        if (command.GetType().FullName?.Contains("Devart.Data.Oracle") == true)
+                        if (this.commandTypeFetcher.Fetch(command) is CommandType commandType)
                         {
-                            string payloadString = payload?.ToString() ?? string.Empty;
-                            string[] result = payloadString.Split([Environment.NewLine], 2, StringSplitOptions.None);
-                            string commandText = result.Length > 1 ? result[1] : payloadString;
-
-                            if (payloadString.Contains("CommandType='Text'"))
+                            if (this.commandTextFetcher.TryFetch(command, out var commandText)) // consider adding here && !string.IsNullOrEmpty(commandText)
                             {
-                                if (this.options.SetDbStatementForText)
+                                switch (commandType)
                                 {
-                                    if (this.options.EmitOldAttributes)
-                                    {
-                                        activity.AddTag(AttributeDbStatement, commandText);
-                                    }
+                                    case CommandType.StoredProcedure:
+                                        if (this.options.SetDbStatementForStoredProcedure)
+                                        {
+                                            if (this.options.EmitOldAttributes)
+                                            {
+                                                activity.AddTag(AttributeDbStatement, commandText);
+                                            }
+
+                                            if (this.options.EmitNewAttributes)
+                                            {
+                                                activity.AddTag(AttributeDbQueryText, commandText);
+                                            }
+                                        }
 
-                                    if (this.options.EmitNewAttributes)
-                                    {
-                                        activity.AddTag(AttributeDbQueryText, commandText);
-                                    }
+                                        break;
+
+                                    case CommandType.Text:
+                                        if (this.options.SetDbStatementForText)
+                                        {
+                                            if (this.options.EmitOldAttributes)
+                                            {
+                                                activity.AddTag(AttributeDbStatement, commandText);
+                                            }
+
+                                            if (this.options.EmitNewAttributes)
+                                            {
+                                                activity.AddTag(AttributeDbQueryText, commandText);
+                                            }
+                                        }
+
+                                        break;
+
+                                    case CommandType.TableDirect:
+                                        break;
+                                    default:
+                                        break;
                                 }
                             }
-                            else if (payloadString.Contains("CommandType='StoredProcedure'"))
+                            else if (command.GetType().FullName?.StartsWith("Devart.Data.Oracle", StringComparison.Ordinal) == true)
                             {
-                                if (this.options.SetDbStatementForText)
+                                string payloadString = payload?.ToString() ?? string.Empty;
+                                string[] result = payloadString.Split([Environment.NewLine], 2, StringSplitOptions.None);
+                                string devartCommandText = result.Length > 1 ? result[1] : payloadString;
+
+                                if (this.options.SetDbStatementForText && payloadString.Contains("CommandType='Text'"))
                                 {
                                     if (this.options.EmitOldAttributes)
                                     {
-                                        activity.AddTag(AttributeDbStatement, commandText);
+                                        activity.AddTag(AttributeDbStatement, devartCommandText);
                                     }
 
                                     if (this.options.EmitNewAttributes)
                                     {
-                                        activity.AddTag(AttributeDbQueryText, commandText);
+                                        activity.AddTag(AttributeDbQueryText, devartCommandText);
                                     }
                                 }
-                            }
-                            else if (payloadString.Contains("CommandType='TableDirect'"))
-                            {
-                            }
-                            else
-                            {
-                            }
-                        }
-                        else if (this.commandTypeFetcher.Fetch(command) is CommandType commandType)
-                        {
-                            var commandText = this.commandTextFetcher.Fetch(command);
-                            switch (commandType)
-                            {
-                                case CommandType.StoredProcedure:
-                                    if (this.options.SetDbStatementForStoredProcedure)
+                                else if (this.options.SetDbStatementForText && payloadString.Contains("CommandType='StoredProcedure'"))
+                                {
+                                    if (this.options.EmitOldAttributes)
                                     {
-                                        if (this.options.EmitOldAttributes)
-                                        {
-                                            activity.AddTag(AttributeDbStatement, commandText);
-                                        }
-
-                                        if (this.options.EmitNewAttributes)
-                                        {
-                                            activity.AddTag(AttributeDbQueryText, commandText);
-                                        }
+                                        activity.AddTag(AttributeDbStatement, devartCommandText);
                                     }
 
-                                    break;
-
-                                case CommandType.Text:
-                                    if (this.options.SetDbStatementForText)
+                                    if (this.options.EmitNewAttributes)
                                     {
-                                        if (this.options.EmitOldAttributes)
-                                        {
-                                            activity.AddTag(AttributeDbStatement, commandText);
-                                        }
-
-                                        if (this.options.EmitNewAttributes)
-                                        {
-                                            activity.AddTag(AttributeDbQueryText, commandText);
-                                        }
+                                        activity.AddTag(AttributeDbQueryText, devartCommandText);
                                     }
-
-                                    break;
-
-                                case CommandType.TableDirect:
-                                    break;
-                                default:
-                                    break;
+                                }
                             }
                         }
 
-- 
5.45.2.windows.1

Sorry for waiting long for a feedback, it is not trivial change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kielek Thank you for the response and suggestions.
When this code is executed this.commandTypeFetcher.Fetch(command) in the IF statement, the exception will leave the EntityFrameworkCoreCommandExecuting case (It will not step into the ELSE IF statement to check if it's Devart). This can be fixed with a try-catch statement, where the TRY part contains the original code, and the CATCH statement handles fallback:

try
{
    if (this.commandTypeFetcher.Fetch(command) is CommandType commandType)
    {
        if (this.commandTextFetcher.TryFetch(command, out var commandText) && !string.IsNullOrEmpty(commandText))
        {
            switch (commandType)
            {
                case CommandType.StoredProcedure:
                    if (this.options.SetDbStatementForStoredProcedure)
                    {
                        if (this.options.EmitOldAttributes)
                        {
                            activity.AddTag(AttributeDbStatement, commandText);
                        }

                        if (this.options.EmitNewAttributes)
                        {
                            activity.AddTag(AttributeDbQueryText, commandText);
                        }
                    }

                    break;

                case CommandType.Text:
                    if (this.options.SetDbStatementForText)
                    {
                        if (this.options.EmitOldAttributes)
                        {
                            activity.AddTag(AttributeDbStatement, commandText);
                        }

                        if (this.options.EmitNewAttributes)
                        {
                            activity.AddTag(AttributeDbQueryText, commandText);
                        }
                    }

                    break;

                case CommandType.TableDirect:
                    break;
                default:
                    break;
            }
        }
    }
}
catch (Exception)
{
    string payloadString = payload?.ToString() ?? string.Empty;
    string[] result = payloadString.Split([Environment.NewLine], 2, StringSplitOptions.None);
    string commandText = result.Length > 1 ? result[1] : payloadString;

    if (payloadString.Contains("CommandType='Text'"))
    {
        if (this.options.SetDbStatementForText)
        {
            if (this.options.EmitOldAttributes)
            {
                activity.AddTag(AttributeDbStatement, commandText);
            }

            if (this.options.EmitNewAttributes)
            {
                activity.AddTag(AttributeDbQueryText, commandText);
            }
        }
    }
    else if (payloadString.Contains("CommandType='StoredProcedure'"))
    {
        if (this.options.SetDbStatementForText)
        {
            if (this.options.EmitOldAttributes)
            {
                activity.AddTag(AttributeDbStatement, commandText);
            }

            if (this.options.EmitNewAttributes)
            {
                activity.AddTag(AttributeDbQueryText, commandText);
            }
        }
    }
}

With this change we don't need to check if Devart is causing this problem or another provider for database connection.
Is this change acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

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

What's about converting it to if (this.commandTypeFetcher.TryFetch(command, out var commandType), without try/catch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried it but still throws exception unfortunately 😞

Copy link
Contributor Author

@vladajankovic vladajankovic Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Here is what I've dug up while analyzing this exception. Firstly, the Fetch function calls the TryFetch function in its implementation. The TryFetch function is implemented as:

public bool TryFetch(object obj, out T value)
{
    if (obj == null)
    {
        value = default;
        return false;
    }

    if (this.innerFetcher == null)
    {
        var type = obj.GetType().GetTypeInfo();
        var property = type.DeclaredProperties.FirstOrDefault(p => string.Equals(p.Name, this.propertyName, StringComparison.OrdinalIgnoreCase)) ?? type.GetProperty(this.propertyName);
        this.innerFetcher = PropertyFetch.FetcherForProperty(property);
    }

    return this.innerFetcher.TryFetch(obj, out value);
}

Now if we follow the code execution, PropertyFetch.FetcherForProperty(property); is called with the following implementation:

public static PropertyFetch FetcherForProperty(PropertyInfo propertyInfo)
{
    if (propertyInfo == null || !typeof(T).IsAssignableFrom(propertyInfo.PropertyType))
    {
        // returns null on any fetch.
        return new PropertyFetch();
    }

    var typedPropertyFetcher = typeof(TypedPropertyFetch<,>);
    var instantiatedTypedPropertyFetcher = typedPropertyFetcher.MakeGenericType(
        typeof(T), propertyInfo.DeclaringType, propertyInfo.PropertyType);
    return (PropertyFetch)Activator.CreateInstance(instantiatedTypedPropertyFetcher, propertyInfo);
}

The return statement calls (PropertyFetch)Activator.CreateInstance(instantiatedTypedPropertyFetcher, propertyInfo); which calls the constructor of the TypedPropertyFetch:

#pragma warning disable CA1812
        private sealed class TypedPropertyFetch<TDeclaredObject, TDeclaredProperty> : PropertyFetch
#pragma warning restore CA1812
            where TDeclaredProperty : T
        {
            private readonly Func<TDeclaredObject, TDeclaredProperty> propertyFetch;

            public TypedPropertyFetch(PropertyInfo property)
            {
#if NET
                this.propertyFetch = property.GetMethod.CreateDelegate<Func<TDeclaredObject, TDeclaredProperty>>();
#else
                this.propertyFetch = (Func<TDeclaredObject, TDeclaredProperty>)property.GetMethod.CreateDelegate(typeof(Func<TDeclaredObject, TDeclaredProperty>));
#endif
            }

When CreateDelegate() is called, the exception is thrown System.ArgumentException: 'Cannot bind to the target method because its signature is not compatible with that of the delegate type.'.

This is the part I can't solve, which is why I made this workaround.

{
string payloadString = payload?.ToString() ?? string.Empty;
if ((payloadString.Contains("CommandType='Text'") && this.options.SetDbStatementForText) ||
(payloadString.Contains("CommandType='StoredProcedure'") && this.options.SetDbStatementForStoredProcedure))
{
string[] result = payloadString.Split(['\n'], 2);
if (result.Length > 1)
{
activity.AddTag(AttributeDbStatement, result[1]);
}
}
}
else if (this.commandTypeFetcher.Fetch(command) is CommandType commandType)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is couple things you should consider here:

  1. Support both for emitting old and new attributes. See lese if statement for details:
                                    if (this.options.SetDbStatementForStoredProcedure)
                                    {
                                        if (this.options.EmitOldAttributes)
                                        {
                                            activity.AddTag(AttributeDbStatement, commandText);
                                        }
                                        if (this.options.EmitNewAttributes)
                                        {
                                            activity.AddTag(AttributeDbQueryText, commandText);
                                        }
                                    }
  1. payloadString split, is it always '\n' or it can be vary based on the environment settings?
  2. Consider adding stub/mocked tests for this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I was writing this code based on an older version (1.0.0-beta.10) that didn't have this.options.EmitOldAttributes and this.options.EmitNewAttributes. So, I will update the code structure to follow the convention of the switch statement.

  2. Updated the line as follows: string[] result = payloadString.Split([Environment.NewLine], 2, StringSplitOptions.None);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I am unable to write mock tests because Devart requires a license in order to connect to Oracle database. 😞

{
var commandText = this.commandTextFetcher.Fetch(command);
switch (commandType)
Expand Down
Loading