Skip to content

Conversation

@GochoMugo
Copy link
Contributor

Which problem is this PR solving?

When using sql-templating libraries such as sql-template-strings, the query object is usually not a plain object, and instead a class instance.

An extracted snippet:

class SQLStatement {
  constructor(strings, values) {
    this.strings = strings
    this.values = values
  }

  /** Returns the SQL Statement for node-postgres */
  get text() {
    return this.strings.reduce((prev, curr, i) => prev + '$' + i + curr)
  }

  setName(name) {
    this.name = name
    return this
  }
}

function SQL(strings) {
  return new SQLStatement(strings.slice(0), Array.from(arguments).slice(1))
}

This class would be instantiated, for example, when using template string:

client.query(SQL`select ...`);

With the changes added in PR #3196, the text property from the Statement class above would not be captured when using the spread operator i.e. { ...arg0 }.

For example,

class Klass {
  constructor() {
    this.foo = 1;
  }
  get bar() {
    return 2;
  }
}
const instance = new Klass();
const obj = { ...instance };

assert(obj.foo === 1);
assert(obj.bar === undefined); // `bar` is not set!

Short description of the changes

The current fix is to explicitly capture name and text props while still using the spread operator.

@GochoMugo GochoMugo requested a review from a team as a code owner November 26, 2025 08:03
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 26, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: GochoMugo / name: Gocho Mugo (802c260)

@GochoMugo GochoMugo force-pushed the fix/pg-instrumentation branch from 1ad024e to b8524b1 Compare November 28, 2025 05:46
@raphael-theriault-swi
Copy link
Member

Hey @GochoMugo, I reviewed #3259 without seeing that you opened this before. However I do prefer the approach you're taking here by still creating a new object as opposed to updating the existing one. Would you be able to rebase your changes on the current version ? There are already new tests covering the non-enumerable property cases so only the instrumentation changes would be needed.

@GochoMugo GochoMugo force-pushed the fix/pg-instrumentation branch from b8524b1 to 802c260 Compare December 6, 2025 04:23
@GochoMugo
Copy link
Contributor Author

Hey @GochoMugo, I reviewed #3259 without seeing that you opened this before. However I do prefer the approach you're taking here by still creating a new object as opposed to updating the existing one. Would you be able to rebase your changes on the current version ? There are already new tests covering the non-enumerable property cases so only the instrumentation changes would be needed.

@raphael-theriault-swi Sure, I have rebased the PR.

Copy link
Member

@raphael-theriault-swi raphael-theriault-swi left a comment

Choose a reason for hiding this comment

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

Much appreciated !

@maryliag
Copy link
Contributor

maryliag commented Dec 8, 2025

@GochoMugo for future reference, don't force push your changes, because then we miss the history and we can't see what exactly changed since latest review :)
thanks for making the changes

@maryliag maryliag merged commit 0b60751 into open-telemetry:main Dec 8, 2025
23 checks passed
@otelbot
Copy link
Contributor

otelbot bot commented Dec 8, 2025

Thank you for your contribution @GochoMugo! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants