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

[Core] DiagnosticSource.Write needs type to work with trimming #37733

Closed
m-redding opened this issue Jul 19, 2023 · 0 comments
Closed

[Core] DiagnosticSource.Write needs type to work with trimming #37733

m-redding opened this issue Jul 19, 2023 · 0 comments
Assignees
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.

Comments

@m-redding
Copy link
Member

m-redding commented Jul 19, 2023

Context

In order to support The Open Telemetry Support for Native AOT as described here, we need to make the DiagnosticScope class compatible with trimming. This is a blocker for doing so.

The internal constructor for DiagnosticScope takes in the parameter object? diagnosticSourceArgs, see L30:

internal DiagnosticScope(string scopeName, DiagnosticListener source, object? diagnosticSourceArgs, ActivitySource? activitySource, System.Diagnostics.ActivityKind kind, bool suppressNestedClientActivities)

And passes it to the private ActivityAdapter class's constructor which sets it to a field object? _diagnosticSourceArgs, see L222:

public ActivityAdapter(ActivitySource? activitySource, DiagnosticSource diagnosticSource, string activityName, System.Diagnostics.ActivityKind kind, object? diagnosticSourceArgs)
{
...
          _diagnosticSourceArgs = diagnosticSourceArgs;

Issue

The issue is in L426 and L529:

_diagnosticSource.Write(_activityName + ".Start", _diagnosticSourceArgs ?? _currentActivity);
_diagnosticSource.Write(_activityName + ".Stop", _diagnosticSourceArgs);

Write() is not trim-friendly because it allows anything to be passed in as the payload. The way to get around this is to use a generic version of write() and preserve the type used for the payload (which is actually introduced in .NET 8+ but we can easily implement the same thing as a method). In addition, the type used in the type parameter needs to be annotated correctly so that the trimmer can preserve the right values.

Examples

There are examples of this here:

Additional Context

Since _diagnosticSourceArgs is of type object, we need to figure out some way to preserve the type being passed in.

The constructor for DiagnosticScope is only used in 2 places outside of tests, one in the DiagnosticScopeFactory, and one in RequestActivityPolicy.

DiagnosticScopeFactory passes null in for diagnosticSourceArgs:

 var scope = new DiagnosticScope(
                                scopeName: name,
                                source: _source,
                                diagnosticSourceArgs: null,
                                activitySource: GetActivitySource(_source.Name, name),
                                kind: kind,
                                suppressNestedClientActivities: _suppressNestedClientActivities);

RequestActivityPolicy passes in HttpMessage

using var scope = new DiagnosticScope("Azure.Core.Http.Request", s_diagnosticSource, message, s_activitySource, System.Diagnostics.ActivityKind.Client, false);

Options

  1. Change diagnosticSourceArgs from object? to HttpMessage?
  2. Annotate HttpMessage and dismiss the warning. Document/Log heavily that if something besides HttpMessage is being passed in then things can get trimmed away (this is not ideal)
  3. Create some kind of args class like the following:
namespace Azure.Core.Pipeline;

internal class DiagnosticSourceArgs
{
    public object Value { get; private set; }
    public void AddValue<T>(T value)
    {
        Value = value;
    }
    public void WriteToSource(DiagnosticSource source, string name)
    {
        source.Write(name, value);
    }

}

Non-options

  1. Making DiagnosticScope generic
@m-redding m-redding added Client This issue points to a problem in the data-plane of the library. Azure.Core labels Jul 19, 2023
@m-redding m-redding self-assigned this Jul 19, 2023
@m-redding m-redding changed the title [Core] Consider adding DiagnosticSourceArgs with generic value to enable trimming [Core] DiagnosticSource.Write needs types to work with trimming Jul 25, 2023
@m-redding m-redding changed the title [Core] DiagnosticSource.Write needs types to work with trimming [Core] DiagnosticSource.Write needs type to work with trimming Jul 25, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Nov 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

No branches or pull requests

1 participant