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

feat(koa): Adds support to ignore a span by its layer name #2028

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MrFabio
Copy link
Contributor

@MrFabio MrFabio commented Mar 20, 2024

Which problem is this PR solving?

  • Adds ignoreLayersName option to instrumentation constructor config to allow layers to be ignored by its name.
  • Currently we have the ignoreLayersType config, this new one will run after this check.
  • It is useful if we want to ignore some layers like logger, allowedMethods, and some ones with no name. It also helps with reducing the number of spans.

Short description of the changes

  • Defines KoaInstrumentationConfig with ignoreLayersName which is an array of string (layer names):
  • Utils module with isLayerNameIgnored to check if a layer should be ignored by the name
  • Modifies _patchLayer to check if the layer is ignored by the config

@MrFabio MrFabio requested a review from a team March 20, 2024 19:36
Copy link

linux-foundation-easycla bot commented Mar 20, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: MrFabio / name: Fábio Gomes (30a395f)

@MrFabio MrFabio force-pushed the feature/koa-ignore branch 3 times, most recently from 4623e18 to 47bed61 Compare March 26, 2024 22:16
@MrFabio MrFabio force-pushed the feature/koa-ignore branch 2 times, most recently from f41b872 to a5b3a6e Compare April 2, 2024 13:49
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.75%. Comparing base (5eb61d8) to head (30a395f).

Files with missing lines Patch % Lines
...lemetry-instrumentation-koa/src/instrumentation.ts 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2028   +/-   ##
=======================================
  Coverage   90.75%   90.75%           
=======================================
  Files         169      169           
  Lines        8018     8040   +22     
  Branches     1632     1638    +6     
=======================================
+ Hits         7277     7297   +20     
- Misses        741      743    +2     
Files with missing lines Coverage Δ
...ode/opentelemetry-instrumentation-koa/src/types.ts 100.00% <ø> (ø)
...ode/opentelemetry-instrumentation-koa/src/utils.ts 100.00% <100.00%> (ø)
...lemetry-instrumentation-koa/src/instrumentation.ts 94.04% <75.00%> (-1.08%) ⬇️

... and 1 file with indirect coverage changes

@MrFabio MrFabio force-pushed the feature/koa-ignore branch 8 times, most recently from 882914e to 57512e3 Compare April 6, 2024 13:55
@MrFabio MrFabio force-pushed the feature/koa-ignore branch 2 times, most recently from 8e3b4df to fe52615 Compare May 9, 2024 14:11
@MrFabio MrFabio force-pushed the feature/koa-ignore branch 4 times, most recently from f0e8dd2 to e2b7711 Compare May 21, 2024 21:38
@MrFabio MrFabio force-pushed the feature/koa-ignore branch 2 times, most recently from ff73355 to 1dce93d Compare May 24, 2024 10:31
@MrFabio MrFabio force-pushed the feature/koa-ignore branch 2 times, most recently from 882121f to f7565e7 Compare June 4, 2024 15:02
@MrFabio MrFabio requested a review from a team as a code owner September 25, 2024 18:22
@MrFabio MrFabio force-pushed the feature/koa-ignore branch 3 times, most recently from 8e7b8fd to e36ad25 Compare October 5, 2024 10:17
@MrFabio MrFabio force-pushed the feature/koa-ignore branch 6 times, most recently from 4966650 to 7c75702 Compare October 15, 2024 13:54
@MrFabio MrFabio force-pushed the feature/koa-ignore branch 2 times, most recently from 791e23f to 35e39f8 Compare October 22, 2024 09:29
@MrFabio MrFabio force-pushed the feature/koa-ignore branch 3 times, most recently from 7d78ab1 to 2bf85a9 Compare November 4, 2024 12:20
@@ -54,6 +54,7 @@ Note that generator-based middleware are deprecated and won't be instrumented.
| Options | Type | Example | Description |
| ------------------ | ----------------------------------- | -------------------- | -------------------------------------------------------------------------------------------------------- |
| `ignoreLayersType` | `KoaLayerType[]` | `['middleware']` | Ignore layers of specified type. |
| `ignoreLayersName` | `string[]` | `['logger']` | Ignore layers with specified names. |
Copy link
Contributor

Choose a reason for hiding this comment

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

@MrFabio What would you think about changing this from ignoreLayersName to ignoreLayers and copying the behaviour of the ignoreLayers config from instrumentation-express, given that instrumentation-express already has a very similar config option with (mostly) the same behaviour?

@@ -36,6 +37,7 @@ export const getMiddlewareMetadata = (
[SEMATTRS_HTTP_ROUTE]: layerPath?.toString(),
},
name: context._matchedRouteName || `router - ${layerPath}`,
layerName: context._matchedRouteName || layerPath?.toString() || '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a strong need to have a separate layerName from the name here? The name is the user-visible value used for the span name (IIUC), so it would be a little confusing to not match against that.

See also the matching handling that instrumentation-express does: supporting a string (exact match), regexp, or function for matching:

const satisfiesPattern = (
constant: string,
pattern: IgnoreMatcher
): boolean => {
if (typeof pattern === 'string') {
return pattern === constant;
} else if (pattern instanceof RegExp) {
return pattern.test(constant);
} else if (typeof pattern === 'function') {
return pattern(constant);
} else {
throw new TypeError('Pattern is in unsupported datatype');
}
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was because this logic ran after the name is mutated. This json is what I have when processing the metadata

{
    "attributes": {
        "koa.name": "/documents",
        "koa.type": "router",
        "http.route": "/documents"
    },
    "name": "router - /documents",
    "layerName": "/documents" <--- original layer name
}

so I used the layerName. I will change it to use the interpolated name instead.

@MrFabio MrFabio force-pushed the feature/koa-ignore branch 3 times, most recently from accecb4 to b49bd40 Compare November 8, 2024 17:21
@MrFabio
Copy link
Contributor Author

MrFabio commented Nov 8, 2024

Updated ✅

);
) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that instrumentation.ts calls isLayerIgnored in two different ways -- once to check the type and once to check the name -- I think it would be cleaner to have two separate isLayerTypeIgnored and isLayerNameIgnored (or whatever names).

For example, I think the current isLayerIgnored has an issue: when it is called without a name: isLayerIgnored(type, config), the code below that calls satisfiesPattern is still called, even though name is undefined. It shouldn't be called. Separating to two functions would make this cleaner.

@MrFabio MrFabio force-pushed the feature/koa-ignore branch 2 times, most recently from ff713f3 to f7f30e3 Compare November 8, 2024 23:52
Comment on lines 70 to 82
assert.strictEqual(
utils.isLayerTypeIgnored(KoaLayerType.MIDDLEWARE, {}),
false
);
assert.strictEqual(
utils.isLayerTypeIgnored(
KoaLayerType.MIDDLEWARE,
{} as KoaInstrumentationConfig
),
false
);
assert.strictEqual(
utils.isLayerTypeIgnored(KoaLayerType.MIDDLEWARE, {
ignoreLayers: {},
} as KoaInstrumentationConfig),
false
);
assert.strictEqual(
utils.isLayerTypeIgnored(KoaLayerType.MIDDLEWARE, {
ignoreLayers: {},
} as KoaInstrumentationConfig),
false
);
assert.strictEqual(
utils.isLayerTypeIgnored(KoaLayerType.MIDDLEWARE, {}),
false
);
assert.strictEqual(
utils.isLayerTypeIgnored(KoaLayerType.MIDDLEWARE, {
ignoreLayers: [],
} as KoaInstrumentationConfig),
false
);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Were these meant to have been adapted for isLayerNameIgnored()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:instrumentation-koa pkg-status:unmaintained:autoclose-scheduled pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants