Allow additionalInstrumentations in TracingInstrumentation

[pshahid]

Why

This PR adds a small feature to the faro.initialize() API that was opened in #711

What

Allow additionalInstrumentations for non-default tracing instrumentations to be added via faro.initialize() instead of re-implementing in a subclass or being forced to re-implement getIgnoreUrls.

Links

Fixes #711

Checklist

• Tests added
• Changelog updated
• Documentation updated

[CLAassistant]

All committers have signed the CLA.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[codecapitano]

Hi @pshahid thanks a lot for contribution, very good idea!

We need to adjust some things.

The getDefaultOTELInstrumentations function doesn't initialize additional instruments.

I would propose something like the following.

1. We can change the additionalInstrumentations property to a function and and inject the ignoreUrls. So users are able to instantiate the additional instruments inside that function, apply instrumentation specific config and can add the ignoreUrls as well.

Example:

// types.ts

export interface TracingInstrumentationOptions {
  // ...
  additionalInstrumentations?: ({ignoreUrls}: {ignoreUrls: Patterns}) => Instrumentation[];
}

2. Then we can add the instruments in the instrumentation class, for example like so:

// instrumenation.ts

 // ...
  const instrumentations = [
      ...getDefaultOTELInstrumentations({
        ignoreUrls: this.getIgnoreUrls(),
        propagateTraceHeaderCorsUrls,
        fetchInstrumentationOptions,
        xhrInstrumentationOptions,
      }),
    ];

    if (options.additionalInstrumentations) {
      instrumentations.push(...options.additionalInstrumentations({ignoreUrls: this.getIgnoreUrls()}));
    }

    registerInstrumentations({
      instrumentations: options.instrumentations ?? instrumentations,
    });
// ...

Usage will then be like this:

// in initiliazeFaro()

//...

new TracingInstrumentation({
        additionalInstrumentations({ignoreUrls}) => ([new FooInstrumentation({ignoreUrls})])
      }),

//...

Wdyt?

[pshahid]

@codecapitano thanks so much for the reply, these are great adjustments! I'll get them incorporated into the change and re-push

[Bruno-DaSilva]

One thing to double check - are there any side effects of instantiating otel instrumentations but then not using them?
We may want to only getDefaultOTELInstrumentations() if options.instrumentations is undefined.

[codecapitano]

One thing to double check - are there any side effects of instantiating otel instrumentations but then not using them? We may want to only getDefaultOTELInstrumentations() if options.instrumentations is undefined.

Good catch Bruno.
Yes we need to take care about that.

For example conceptually something like that:

registerInstrumentations({
      instrumentations: options.instrumentations ?? [
        ...getDefaultOTELInstrumentations({
          ignoreUrls: this.getIgnoreUrls(),
          propagateTraceHeaderCorsUrls,
          fetchInstrumentationOptions,
          xhrInstrumentationOptions,
        }),
        ...(options.additionalInstrumentations?.({ignoreUrls: this.getIgnoreUrls()}) ?? []),
      ],
    });

Note:
I am on my way home from an offsite so answers might be delayed.

Cheers

[codecapitano]

Hi @pshahid, @Bruno-DaSilva

Thank you so much for the contribution.
While looking and testing the PR it came to me that the PR revealed some sub optimal API design of the TracingInstrumentation class which I think it may make sense to tackle.

With the current API design we have properties which affect default and custom instruments as well as only custom instruments.

I think this is very straight forward and creates ambiguity.

But your idea is super valuable so I have some ideas I would like to discuss.
And don't worry, we can either work together on this or if it's to much time to spend for you on top of your daily work, then the Faro team can take care of it.

Look at the following example:

• It's not clear that instrumentationOptions only affects the default instruments.
  Maybe we can deprecate the name, provide a second prop which is call something like defaultInstrumentationOption and read them both under the good.
• instruments: [] and additionalInstruments: () => [] are very similar.
  Why not export getIgnoreURLs or make the instrumentations property work with instrumentations[] | ({ignoreUrls}: {ignoreUrls: Patterns}) => Instrumentation[];.
  If a function is provided then call it with the ignoreUrls a param.

I bet you have some good ideas as well so please don't hesitate and add them :)

Current config with comments showing the situation:

      new TracingInstrumentation({
        // Affects both, default and custom instruments
        contextManager: ...,
        propagator: ...,
        resourceAttributes: {},
        spanProcessor: ...,

        // Affects only default instruments
        // -- maybe deprecate and rename to `defaultInstrumentationOptions`?
        instrumentationOptions: {
          fetchInstrumentationOptions: {
            applyCustomAttributesOnSpan: () => {},
            ignoreNetworkEvents: true,
          },
          xhrInstrumentationOptions: {
            applyCustomAttributesOnSpan: () => {},
            ignoreNetworkEvents: true,
          },
        },

        // affects custom and default instruments
        // -- it's not directly clear what's the difference between `instrumentations` and `additionalInstrumentations`. When do I need to use what?
        // ---- get rid of it.
        additionalInstrumentations: (ignoreUrls) => [],

        // Overwrites default instruments allows to add more instruments via the `additionalInstrumentations` function
        // -- Why not mak instrumentation to be working with both:
        // --- Tou can provide ad array of instruments or use a function like additionalInstrumentations() to provide an array of instruments
        instrumentations: [], // instrumentations[] | instrumentations: (ignoreUrls) => instrumentations[],
      }),

@cedricziel, @kpelelis, @eskirk Would love to get your input as well.

Wdyt and again big thanks,
Marco

[Bruno-DaSilva]

@codecapitano quick q - how do y'all feel about breaking changes? Do we need to only consider changes that are backwards compatible with existing behaviour?

[codecapitano]

@codecapitano quick q - how do y'all feel about breaking changes? Do we need to only consider changes that are backwards compatible with existing behaviour?

It depends a bit on the change and it's impact.
Usually we try to avoid breaking changes of course but on rare occasions we still had them.

For example in our experimental modules or if they inevitable, like when we introduced the new session manager or if the impact is super low but impact is high. All of this need discussion with the team first.

Introducing breaking changes is especially problematic for users using the CDN version because, in it's default config, it auto fetches minor and patch updates on page load.

---

So by the end it needs to be done on a case by case decision.

[pshahid]

@codecapitano I agree that the approach suggested creates some ambiguity about how to handle override vs additional instrumentation. If you already have ideas on how to move forward with changing the API then please proceed without this contribution, or feel free to rework it as you see fit.

For me personally, I need to keep chugging on daily work to meet some monthly/quarterly goals, so I likely won't have enough time to collaborate on this change.

If it's as simple as changing instrumentations from an array to an optional array or function I can do that in this PR, but that'd probably be as far as I go for now due to time constraints.

[codecapitano]

Hi @pshahid totally understandable.
Nevertheless you've started this great discussion.
I like the idea a lot and the Faro team can continue the work.

We'll likely create a new branch/PR but of course will add/mention you in this PR to have a reference to your work.

Thanks a lot for kicking this off 🙏

[pshahid]

@codecapitano all good, really appreciate you taking the idea and running with it. We'll keep our eyes on the future PRs/work here. We really appreciate your openness and flexibility here.

Let me know if I should close this PR and/or the issue associated.

[Bruno-DaSilva]

@codecapitano cc @pshahid anything we can do short term to address the needs of this PR, while the more fundamental redesign changes are made separately?

[codecapitano]

@pshahid @Bruno-DaSilva

I think the easiest solution for now would be to expose the ignoreURLs to make it easier to use them in additional otel instrumentations.

It's not the 100% best solution but I think it can already make the setup a bit easier.

Thoughts?

[Bruno-DaSilva]

sounds reasonable to me! Minimizes change to the existing API for now.

[codecapitano]

Hi @pshahid and @Bruno-DaSilva PR to export ignoreUrls is in review.

#732

[codecapitano]

#732

[codecapitano]

@pshahid We released a new Faro version today which exports a getIgnoreUrls function.

import  { getIgnoreUrls } from '@grafana/faro-web-sdk';

[Bruno-DaSilva]

sickkkk thank you @codecapitano!!

[pshahid]

Amazing, thank you @codecapitano!!!