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

instrumentation-pg requestHook isn't called for connection events #2118

Open
drob opened this issue Apr 19, 2024 · 1 comment
Open

instrumentation-pg requestHook isn't called for connection events #2118

drob opened this issue Apr 19, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@drob
Copy link

drob commented Apr 19, 2024

What version of OpenTelemetry are you using?

@opentelemetry/api 1.6.0
@opentelemetry/instrumentation-pg 0.36.1

What version of Node are you using?

18.18.0

What did you do?

You can reproduce this with a simple instrumentation-pg implementation.

const pgInstrumentation = new PgInstrumentation({
  requestHook: (span: Span, requestInfo: PgRequestHookInformation) => {
    // This will appear on query spans, but not connection spans.
    span.setAttribute('library', 'pg');
  },
});

...

registerInstrumentations({
  tracerProvider: provider,
  instrumentations: [
    pgInstrumentation,
  ],
});

What did you expect to see?

requestHook to be called for all pg spans

What did you see instead?

requestHook is called for pg queries but not connection events

@drob drob added the bug Something isn't working label Apr 19, 2024
@drob
Copy link
Author

drob commented Apr 24, 2024

I'd be happy to send a PR for this. Just let me know how you'd want it to work? The request hook for connection events needs to have different args, since the usual request hook knows the query that the client is running, whereas for connection events there isn't a query.

So we could either make the query part optional, like this:

  export interface PgRequestHookInformation {
-   query: {
+   query?: {
      text: string;
      name?: string;
      values?: unknown[];
    };
    connection: {
      database?: string;
      host?: string;
      port?: number;
      user?: string;
    };
  }

Or, we could create a separate type and extend the instrumentation config, like this:

  export interface PgInstrumentationConfig extends InstrumentationConfig {
    ...
+   /**
+    * Hook that allows adding custom span attributes based on the data about a
+    * connection request.
+    *
+    * @default undefined
+    */
+   connectionRequestHook?: PgInstrumentationConnectRequestHook;
    ...
  }

+ export interface PgConnectRequestHookInformation {
+   connection: {
+     database?: string;
+     host?: string;
+     port?: number;
+     user?: string;
+   };
+ }
+
+ export interface PgInstrumentationConnectRequestHook {
+   (span: api.Span, connectionInfo: PgConnectRequestHookInformation): void;
+ }

The latter preserves backcompat and is probably more in line with what a typical user would want, so I'd lean towards that, but I'm happy to implement either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant