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(instrumentation): add util to execute span customization hook in base class #4663

Merged
merged 20 commits into from
May 6, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
c55be5e
feat(instrumentation): hoist span event hook execution to base class
blumamir Apr 27, 2024
a0e2a8d
test: add test for new hook runner
blumamir Apr 28, 2024
d0cd4d5
chore: changelog
blumamir Apr 28, 2024
1c059fe
fix: use event name from arguments
blumamir Apr 28, 2024
c33ba1a
fix: remove unused import
blumamir Apr 28, 2024
944a224
fix: make diag message structual
blumamir Apr 28, 2024
cac3766
Merge branch 'main' into hook-executer
blumamir Apr 29, 2024
fa1a53e
make the private function start with underscore
blumamir Apr 30, 2024
9dc607c
Merge remote-tracking branch 'origin/hook-executer' into hook-executer
blumamir Apr 30, 2024
9f44c05
Merge branch 'main' into hook-executer
blumamir May 1, 2024
d29bda6
Merge remote-tracking branch 'upstream/main' into hook-executer
blumamir May 3, 2024
86fb72a
Merge branch 'hook-executer' of github.com:blumamir/opentelemetry-js …
blumamir May 3, 2024
c2d7518
chore: rename insetrumentation event to span customization hook
blumamir May 3, 2024
e737b7f
chore: update changelog
blumamir May 3, 2024
68c73ba
chore: lint fix
blumamir May 3, 2024
1540713
Update experimental/packages/opentelemetry-instrumentation/src/instru…
blumamir May 6, 2024
9d0daf9
chore: move CHANGELOG to experimental
blumamir May 6, 2024
2035a34
Merge remote-tracking branch 'upstream/main' into hook-executer
blumamir May 6, 2024
73b1308
Merge remote-tracking branch 'origin/hook-executer' into hook-executer
blumamir May 6, 2024
d4e2f38
fix: changelog
blumamir May 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/

### :rocket: (Enhancement)

* feat(instrumentation): add util to execute instrumentation event hook in base class [#4663](https://github.com/open-telemetry/opentelemetry-js/pull/4663) @blumamir

### :bug: (Bug Fix)

* fix(core): align inconsistent behavior of `getEnv()` and `getEnvWithoutDefaults()` when a `process` polyfill is used [#4648](https://github.com/open-telemetry/opentelemetry-js/pull/4648) @pichlermarc
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,15 @@ import {
trace,
Tracer,
TracerProvider,
Span,
} from '@opentelemetry/api';
import { Logger, LoggerProvider, logs } from '@opentelemetry/api-logs';
import * as shimmer from 'shimmer';
import {
InstrumentationModuleDefinition,
Instrumentation,
InstrumentationConfig,
InstrumentationEventHook,
} from './types';

/**
Expand Down Expand Up @@ -173,4 +175,33 @@ export abstract class InstrumentationAbstract implements Instrumentation {
| InstrumentationModuleDefinition
| InstrumentationModuleDefinition[]
| void;

/**
* Execute instrumentation event hook, if configured, and log any errors.
* Any semantics of the event type and eventInfo are defined by the specific instrumentation.
* @param hookHandler The optional hook handler which the user has configured via instrumentation config\
blumamir marked this conversation as resolved.
Show resolved Hide resolved
* @param eventName The name of the event being triggered, for logging purposes
* @param span The span to which the hook should be applied
* @param eventInfo The event info to be passed to the hook
*/
protected runInstrumentationEventHook<EventInfoType>(
Copy link
Member Author

Choose a reason for hiding this comment

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

This function uses this._diag to silently log any errors from hooks. thus the function needs to have access to this and is defined as a class method.

It is marked protected so that we can only run it from a class method but it is not exposed on the public API, similarly to the existing _wrap, _unwrap etc functions.

Flarna marked this conversation as resolved.
Show resolved Hide resolved
hookHandler: InstrumentationEventHook<EventInfoType> | undefined,
eventName: string,
span: Span,
Flarna marked this conversation as resolved.
Show resolved Hide resolved
eventInfo: EventInfoType
) {
if (!hookHandler) {
return;
}

try {
hookHandler(span, eventInfo);
Copy link
Member Author

@blumamir blumamir Apr 28, 2024

Choose a reason for hiding this comment

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

I was wondering if it can be useful to also always include the eventName as an attribute in the event info, something like:

Suggested change
hookHandler(span, eventInfo);
hookHandler(span, {eventName, ...eventInfo});

and adjust the types.

The hook can always choose to ignore this parameter but it will be available if needed. Not sure thought if it's worth adding more complications to the code for a non existing use that I'm aware of at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

This would be helpful if a hook function is used for several hooks. I have no usecase in mind. Price is the extra object created.

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking as well. thanks. will wait for more opinions.

We can also add it in the future in a non breaking manner

} catch (e) {
this._diag.error(
`Error running instrumentation event hook for event due to exception in handler`,
{ eventName },
e
);
Copy link
Member Author

Choose a reason for hiding this comment

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

As part of this PR, I also transformed the diag message to be more structured - make the log message constant and add the eventName as an argument so it is easy to consume.

I believe these type of diag prints are better and should be preferred, but am open to address this in a different PR

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import { TracerProvider, MeterProvider } from '@opentelemetry/api';
import { TracerProvider, MeterProvider, Span } from '@opentelemetry/api';
import { LoggerProvider } from '@opentelemetry/api-logs';

/** Interface Instrumentation to apply patch. */
Expand Down Expand Up @@ -127,3 +127,24 @@ export interface InstrumentationModuleDefinition {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
unpatch?: (moduleExports: any, moduleVersion?: string) => void;
}

/**
* InstrumentationEventHook is a common way for instrumentations to expose extension points
* where users can add custom behavior to a span.
* This is an advanced feature, commonly used to add additional or non spec compliant attributes to the span,
* capture payloads, modify the span in some way, or carry some other side effect.
*
* The hook is registered with the instrumentation specific config by implementing an handler function with this signature,
* and if the hook is present it will be called with the span and the event information
* when the event is emitted.
*
* The event semantics - when and under what conditions the hook is called and what information is passed
* in the eventInfo object, is specific to each instrumentation and event and should be documented
* where it is used.
*
* Instrumentation may define multiple hooks, for different spans, or different events on a span.
*/
export type InstrumentationEventHook<EventInfoType> = (
Copy link
Member Author

Choose a reason for hiding this comment

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

I chose to make the generic type EventInfoType required, to promote typings of the object.

Since some instrumentations are not able to type the object correctly, which might require type export from the instrumented package itself, it is not always possible. In this case the instrumentation can choose to use any in the hook, which communicates that the type was ignored on purpose

Copy link
Member Author

Choose a reason for hiding this comment

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

@open-telemetry/javascript-approvers This PR also introduces a new name: "InstrumentationEvent". At the moment, instrumentation has partially settled on a hook function signature and behaviour. The pattern is a "{some event name}Hook" function that receive a span along with info object, at specific times in the lifecycle of a span it records, and the hook is used to add additional attributes.

I wanted to document it as a type and give it a name that we can use which will promote consistency and reduce mental load for instrumentation writers and maintainers.

WDYT about the name "InstrumentationEvent"? It can only be used in the context of a specific open span (for which we can add attributes), required info object, and no return value.

blumamir marked this conversation as resolved.
Show resolved Hide resolved
span: Span,
eventInfo: EventInfoType
) => void;
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
Instrumentation,
InstrumentationBase,
InstrumentationConfig,
InstrumentationEventHook,
InstrumentationModuleDefinition,
} from '../../src';

Expand All @@ -36,6 +37,12 @@ class TestInstrumentation extends InstrumentationBase {
override enable() {}
override disable() {}
init() {}

// the runInstrumentationEventHook, so we have to invoke it from the class for testing
testRunHook(hookHandler?: InstrumentationEventHook<any>) {
const span = this.tracer.startSpan('test');
this.runInstrumentationEventHook(hookHandler, 'test', span, {});
}
}

describe('BaseInstrumentation', () => {
Expand Down Expand Up @@ -182,5 +189,30 @@ describe('BaseInstrumentation', () => {

assert.deepStrictEqual(instrumentation.getModuleDefinitions(), []);
});

describe('runInstrumentationEventHook', () => {
it('should call the hook', () => {
const instrumentation = new TestInstrumentation({});
let called = false;
const hook = () => {
called = true;
};
instrumentation.testRunHook(hook);
assert.strictEqual(called, true);
});

it('empty hook should work', () => {
const instrumentation = new TestInstrumentation({});
instrumentation.testRunHook(undefined);
});

it('exception in hook should not crash', () => {
const instrumentation = new TestInstrumentation({});
const hook = () => {
throw new Error('test');
};
instrumentation.testRunHook(hook);
});
});
});
});