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

Added configurable activity tracing #541

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

Conversation

mfuqua3
Copy link

@mfuqua3 mfuqua3 commented Sep 11, 2023

This pull request addresses issue #451

It adds configuration support for Wolverine tracing, and adds an additional package for OpenTelemetry instrumentation.

  • Adds configuration support for activity filtering predicates for the three supported Wolverine activity types
  • Adds configuration support for global activity filtering, regardless of origin type
  • Add support of enrichment of Wolverine generated activities
  • Adds first class support for filtering out internal message types
  • Adds additional package for OpenTelemetry instrumentation
Host.CreateDefaultBuilder()
            .UseWolverine(options =>
            {
                // Suppress activity generation for IInternalMessage implementations
                options.Tracing.SuppressInternalMessageTypes = true; 
                options.Tracing.Enrich = (activity, eventType, envelope) =>
                {
                    // enrich the activity externally
                };
                options.Tracing.SendEnvelopeFilter = (envelope) =>
                {
                    //specify a condition in which activities shouldn't be traced for sent messages
                    return true;
                };
                options.Tracing.GlobalFilter = (envelope) =>
                {
                    //Specify a condition in which activities shouldn't be traced for any message
                    return true;
                };
            })

-- OR --

Host.CreateDefaultBuilder()
            .UseWolverine()
            .ConfigureServices(services =>
            {
                services.AddOpenTelemetry()
                    .WithTracing(builder =>
                    {
                        builder
                            .SetResourceBuilder(ResourceBuilder.CreateDefault().AddService("MY SERVICE"))
                            .AddWolverineInstrumentation(options =>
                            {
                               // Suppress activity generation for IInternalMessage implementations
                                options.SuppressInternalMessageTypes = true; 
                                options.Enrich = (activity, eventType, envelope) =>
                                {
                                    // enrich the activity
                                };
                                options.SendEnvelopeFilter = (envelope) =>
                                {
                                    //specify a condition in which activities shouldn't be traced for sent messages
                                    return true;
                                };
                                options.GlobalFilter = (envelope) =>
                                {
                                    //specify a condition in which activities shouldn't be traced for any messages
                                    return true;
                                };
                            });
                    });
            });

@jeremydmiller
Copy link
Member

jeremydmiller commented Sep 13, 2023

@mfuqua3 Hey, Matt, I'm going to get this in soon, but with a couple minor changes. I'm going to introduce a "composite filter" pattern so you can register separate includes/excludes. Experience says you'd want that and those one size fits all Func<something, bool> is never perfectly usable.

The other minor things I'm going to do is pull this:

            if (!WolverineActivitySource.Options.GetFilterForActivity(spanName).Invoke(envelope))
            {
                logger.LogTrace("Execution of envelope {EnvelopeId} is filtered out from activity tracing", envelope.Id);
                return null;
            }

into each individual Start**** method just to eliminate the lookup. It's a micro-optimization, but you work under the assumption that everything a framework does is the hot path.

Question for you, how do you see the Enrich() being used? My intention was that folks would mostly use middlware or even explicit code in handlers for that (plus that's already supported in no small part because of repetitive code I saw in your giant monolith).

Comments aside, thank you for doing this work, I know folks are going to appreciate this one!

@mfuqua3
Copy link
Author

mfuqua3 commented Sep 13, 2023

The other minor things I'm going to do is pull this into each individual Start**** method just to eliminate the lookup. It's a micro-optimization, but you work under the assumption that everything a framework does is the hot path.

Makes sense. I actually went back and forth initially on whether the code reuse was worth it, so I'm with you there.

Question for you, how do you see the Enrich() being used?

So I do not necessarily have an immediate need for this capability. I actually borrowed this concept (and most of the contract for this options class) from another OpenTelemetry instrumentation package -- the WCF one to be specific. I do not anticipate I would use this feature right away, if ever. Although I can anticipate use cases for it:

  1. I figure it provides an extensibility hook for Activity tagging from the Core library that could be utilized by other Wolverine extensions, especially if they aren't first-party extensions that might have access to the internal API.
  2. I think it provides an easy way to add interop tags as a Wolverine configuration, or to rename tags to match legacy names so Wolverine generated traces can be included in existing trace queries (maybe I need the tenant tag to be named tenancy instead of tenant.id for example)

Also, making it part of the options class allows it to be easily configured in an AddWolverineInstrumentation() call. This could obviously still be done using middleware, but I'm not sure if I like the idea of an OpenTelemetry configuration call adding Wolverine middleware under the hood. Developers might not anticipate it, and therefore not "order" it correctly, and it could have unintended consequences based on its execution timing.

@jeremydmiller
Copy link
Member

@mfuqua3 Hey Matt, I did take a very close look at this, but decided to do the Otel filtering (and more logging filtering in general) a bit different way -- mostly out of micro-optimization thinking. I will revisit this in 1.8 and use the new extension that adds Otel to Wolverine. That one's a winner. I'll also try to do 1.8 much sooner than the gap was between 1.6 and 1.7

@mfuqua3
Copy link
Author

mfuqua3 commented Sep 22, 2023

Roger that, sounds good!

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.

2 participants