chore: updated instrumentations with safety check on promises

[abhilash-sivan]

refs https://jsw.ibm.com/browse/INSTA-70534

This PR is an extension of the following case: #2249

"An internal error happend in the Instana Node.js collector. Please contact support. TypeError: Cannot read properties of undefined (reading 'then')
at /opt/instana/instrumentation/nodejs/node_modules/@instana/core/src/tracing/instrumentation/databases/redis.js:349:28"

The possible cause for this TypeError is a sync error, so we have to add a promise-type safety check in the missing instrumentations, and this PR handles that.

Implementation

• Added condition on the unhandled cases and marked span as incomplete
• The UI will show custom tag as following (it's an example, not the exact case)

[abhilash-sivan]

else case added for case:
If this is not a promise, then end the span gracefully

[kirrg001]

AFAIK there is three cases:

1. promise (mostly default)
2. callback (if lib supports callbacks)
3. sync error (try/catch)

I assume (3) was the cause of #2249?

[abhilash-sivan]

Discussed. The 'else' case is not valid for sync error; updated accordingly.

[kirrg001]

There are 5 cases:

1. Promise (mostly default)
2. Callback (if lib supports callbacks)
3. Normal sync calls (usually non of our libraries uses sync calls)
4. Validation error thrown synchronously (try/catch) -> does not even reach promise.then
5. Unsupported/Bug case in our instrumentation or library bug

Example for unsupported case: #2295
In this case our instrumentation had a bug where we handle the original function wrong and this resulted in an undefined return value.

For 4:
We should always send the span with the actual sync error. We do that already most of the time. Please double check for the code you have touched.

For 5:
IMO we should add a debug log (anything else could be too noisy) and we should always send the span, but with an information attached to the span? Because we miss the duration, result and errors. TBD

[kirrg001]

Cool.

How about we add a utility?

tracingUtil....(...)

[abhilash-sivan]

Cool.

How about we add a utility?

tracingUtil....(...)

for the following case right ?

else case added for case:
If this is not a promise, then end the span gracefully

[abhilash-sivan]

Cool.

How about we add a utility?

tracingUtil....(...)

This doesn’t seem very useful at the moment, because we already check for the case promise?.then === 'function' and proceed accordingly. Introducing a new common utility would keep the same checks, but would only add an extra function call without simplifying the logic.

It also makes sense to leave the logic as-is, since it allows us to customize the instrumentation behavior on a case-by-case basis if needed. Centralizing this logic in a common utility doesn’t appear to make things simpler, in my opinion.

[abhilash-sivan]

Quality Gate failed

Failed conditions 76.8% Coverage on New Code (required ≥ 80%) 7.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Only added some type checks. After merging, the coverage will be >85%

[kirrg001]

This doesn’t seem very useful at the moment, because we already check for the case promise?.then === 'function' and proceed accordingly. Introducing a new common utility would keep the same checks, but would only add an extra function call without simplifying the logic.

Yeah fine with me 👍 We can also keep as it is

[sonarqubecloud]

Quality Gate failed

Failed conditions
40.1% Coverage on New Code (required ≥ 80%)
9.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud