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

New input: ActivitySource #364

Closed
WhitWaldo opened this issue Jul 28, 2020 · 12 comments
Closed

New input: ActivitySource #364

WhitWaldo opened this issue Jul 28, 2020 · 12 comments
Labels
new-feature New feature requests

Comments

@WhitWaldo
Copy link

I've been reading a good deal about OpenTelemetry and the plans to use System.Diagnostics.ActivitySource as the basis for the .NET implementation. I've read through both the DiagnosticSource user guide and the ActivitySource user guide and I understand that activities can be created as part of a DiagnosticSource in the 4.7 release, but given the .NET 5 preview (and that first link up there), it looks more as though ActivitySource is intended to be used independently of DiagnosticSource with OpenTelemetry.

To that end, I looked into what's necessary to create an input to EventFlow for ActivitySource and it looks like it can be nearly identical to DiagnosticSource except for this file, lines 56 through 65 since there's no name property on the listener. I believe I've gathered that the point of this is so it's hooking up subscribers on listener matches.

There's a listener name on the ActivitySource, but I don't see any of way accessing it from the ActivityListener? Do you have any ideas on this so I might add this input (absent DiagnosticSource)?

Thank you!

@karolz-ms karolz-ms added under-investigation Need more info to determine whether there is a problem and what to do about it new-feature New feature requests and removed under-investigation Need more info to determine whether there is a problem and what to do about it labels Jul 28, 2020
@karolz-ms
Copy link
Collaborator

Whit, thank you for bringing this to our attention.

The hook up of ActivitySource looks simple enough, very similar to DiagnosticSource, like you said. There is quite a bit of new data exposed by ActivitySource events, so we probably need to spend some time familiarizing ourselves with the new API and determining what is the best way to translate the data available into the flat data structure that EventFlow's EventData uses.

For future reference:
ActivitySource design document part 1
ActivitySource design document part 2
GitHub issue with further design discussion
Document comparing OpenTelemetry and .NET Tracing API

@karolz-ms
Copy link
Collaborator

Looking at hookup issue some more, looks like the details are being worked out still and are somewhat different from DiagnosticSource / DiagnosticListener.

One of the OpenTelemetry core goals is to be able to listen to arbitrary set of activity sources; this is exactly what EventFlow needs too, so I am not too worried about details changing, but it probably makes sense to wait a bit till the implementation stabilizes, and only then look into adding ActivitySourceInput to EventFlow.

@WhitWaldo
Copy link
Author

WhitWaldo commented Aug 5, 2020

For visibility, this Medium post indicates that the OpenTelemetry .NET package has entered beta, based on the .NET Activity API.

It indicates that System.Diagnostics.DiagnosticSource will be in a preview state until November 2020, presumably meaning it will GA alongside .NET 5.

@karolz-ms
Copy link
Collaborator

@WhitWaldo, thank you!

@karolz-ms
Copy link
Collaborator

This will probably have to wait till .NET 5.0 GA and the associated VS tooling catches up. ActivitySource is .NET 5.0-only; as of right now VS 2019 setup does not include .NET 5.0 SDK, so we won't be able to produce signed builds with Azure DevOps pipelines.

@WhitWaldo
Copy link
Author

@karolz-ms Just following up on this now that 5.0 is GA. Since I'm using Service Fabric and it won't have .NET 5.0 support until this spring, this isn't a high priority yet, but it'd be nice to see it supported around the time I'm able to use it myself.

@karolz-ms
Copy link
Collaborator

@WhitWaldo yep. We'll get this taken care of by end of this calendar year.

@ghelyar
Copy link
Contributor

ghelyar commented Jan 4, 2021

I wrote the DiagnosticSource input for EventFlow, and I have started moving towards using OpenTelemetry and ActivitySource instead.

If you do want to use ActivitySource with EventFlow, here's a quick ActivitySource sample that highlights a few key parts:

using (var listener = new ActivityListener
{
    ActivityStopped = activity =>
    {
        Console.WriteLine(new
        {
            activity.Recorded,

            activity.IdFormat,
            activity.Id,
            activity.Context.TraceId,
            activity.Context.SpanId,
            activity.ParentId,
            activity.ParentSpanId,

            activity.StartTimeUtc,
            activity.Duration,

            activity.OperationName,
            activity.DisplayName,

            activity.Kind,
            Bags = string.Join(";", activity.Baggage.Select(t => t.Key + ":" + t.Value)),
            Tags = string.Join(";", activity.TagObjects.Select(t => t.Key + ":" + t.Value)), // TagObjects is a superset of Tags

            Events = string.Join(",", activity.Events.Select(e => new
            {
                e.Name,
                e.Timestamp,
                Tags = string.Join(";", e.Tags.Select(t => t.Key + ":" + t.Value))
            })),
            Links = string.Join(",", activity.Links.Select(l => new
            {
                l.Context.TraceId,
                l.Context.SpanId,
                Tags = string.Join(";", l.Tags.Select(t => t.Key + ":" + t.Value))
            })),
            CustomProperty = activity.GetCustomProperty("prop"),
            
            SourceName = activity.Source.Name,
            SourceVersion = activity.Source.Version
        });
    },
    ShouldListenTo = source => source.Name == "my source",
    Sample = (ref ActivityCreationOptions<ActivityContext> options) => ActivitySamplingResult.AllDataAndRecorded
})
{
    ActivitySource.AddActivityListener(listener);

    Activity.DefaultIdFormat = ActivityIdFormat.W3C;
    using (var source = new ActivitySource("my source", "1.2.3"))
    {
        using (var activity = source.StartActivity("my activity"))
        {
            try
            {
                if (activity != null)
                {
                    activity.AddBaggage("bag", "value");
                    activity.AddTag("tag.string", "value");
                    activity.AddTag("tag.object", new { key = "value" });
                    activity.SetCustomProperty("prop", new { foo = "bar" });
                    throw new Exception("test");
                }
            }
            catch (Exception ex) when (activity != null)
            {
                // Add the OpenTelemetry.Api package and import the OpenTelemetry.Trace namespace to record exceptions in this way.
                activity.RecordException(ex);
            }
        }
    }
}

Also note that all you need is the System.Diagnostics.DiagnosticSource package version 5.0 or higher, you don't need to actually use .NET 5.0. It will work on older versions of .NET Core or on the full .NET Framework. There is, however, a bug in 5.0.0 where timestamps are not high precision for .NET Framework, the fix for which is due to be released with 5.0.1 of the package.

This also shows how to filter which ActivitySources you listen to by name, which was part of the question above. You can also choose to only sample specific activities, while this example samples everything and then only listens to a single source by name. When an activity is not sampled, StartActivity will return null.

I think it's different enough that probably needs a completely separate input for EventFlow, but it's quite easy to make a custom EventFlow input. You just need to work out how to translate an Activity into an EventData. You can always make your own custom input, then when you're happy with it, submit it to here. That's what I did with DiagnosticSource.

@karolz-ms
Copy link
Collaborator

Thank you, @ghelyar I am getting close to submitting a PR with the new input, working on tests now https://github.com/karolz-ms/diagnostics-eventflow/tree/dev/karolz/activity-source

@ghelyar
Copy link
Contributor

ghelyar commented Jan 4, 2021

FWIW I think the serialization should be the responsibility of the output, not the input, but you're the expert. It would make it harder to do things like extract exceptions from events in the output if it's already serialized, and the output might want to serialize it differently anyway. Part of the reason I originally moved from EventSource to DiagnosticSource was to defer serialization.

Also, you don't need to read both Tags and TagObjects, because TagObjects contains all Tags.

You can also generate the dictionary of activity kind names once rather than either hard coding it or using Enum.GetName/ToString every time:

private static readonly IReadOnlyDictionary<ActivityKind, string> ActivityKindNames =
    Enum.GetValues(typeof(ActivityKind)).Cast<ActivityKind>().ToDictionary(k => k, k => k.ToString());

@karolz-ms
Copy link
Collaborator

@ghelyar great points, I agree, thank you. I will make the changes you suggest.

karolz-ms added a commit to karolz-ms/diagnostics-eventflow that referenced this issue Jan 7, 2021
Fixes issue Azure#364

Includes re-organization of the README file to make navigation easier
karolz-ms added a commit to karolz-ms/diagnostics-eventflow that referenced this issue Jan 12, 2021
Fixes issue Azure#364

Includes re-organization of the README file to make navigation easier
karolz-ms added a commit that referenced this issue Jan 15, 2021
Fixes issue #364

Includes re-organization of the README file to make navigation easier
@karolz-ms
Copy link
Collaborator

@WhitWaldo @ghelyar this has been published on Nuget. Enjoy! ☺️

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

No branches or pull requests

3 participants