Skip to content

Conversation

@marandaneto
Copy link
Contributor

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

🔮 Next steps

@marandaneto
Copy link
Contributor Author

Tests fail due to:

FAIL test/tracing/stalltracking.test.ts (6.517s)
● StallTracking › Stall tracking detects a JS stall
TypeError: hub$1.getClient is not a function
52 | setTimeout(() => {
53 | stallTracking.onTransactionFinish(transaction);
> 54 | transaction.finish();
| ^
55 |
56 | const measurements = getLastEvent()?.measurements;
57 |
at Transaction._getBaggageWithSentryValues (node_modules/@sentry/tracing/cjs/span.js:345:24)
at Transaction.getBaggage (node_modules/@sentry/tracing/cjs/span.js:304:16)
at Transaction.finish (node_modules/@sentry/tracing/cjs/transaction.js:119:51)
at Timeout._onTimeout (test/tracing/stalltracking.test.ts:54:19)
at listOnTimeout (node:internal/timers:559:17)
at processTimers (node:internal/timers:502:7)

@marandaneto
Copy link
Contributor Author

were there any breaking changes related to it?

@Lms24
Copy link
Member

Lms24 commented Jun 22, 2022

Hmm this is weird.. I took a quick look but afaict we didn't change anything w.r.t the hub API or in anything publicly exposed in the Span class.

This line should be, where the test currently fails, right?:
https://github.com/getsentry/sentry-javascript/blob/45818f3348ce8dc30d22743b2093f5a2aebb1ce9/packages/tracing/src/span.ts#L373

So for some reason, there seems to be a problem with hub. Any chance that it is not correctly set when starting/creating the transaction? I can't recall changing anything there either, though.

EDIT: I'm currently doing some more work in this area so I removed the sketchy type cast from _getBaggageWithSentryValues and made sure to only call hub.getClient() when the hub actually exists. But the error you get doesn't seem to be caused by an undefined client but rather by a malformed one...

@marandaneto
Copy link
Contributor Author

marandaneto commented Jun 22, 2022

Yes, exactly that line of the code, wondering if it's something that JS SDK has to fix or should I should mock it in my tests.

Edit: worked if I mock getClient ad94e98

@Lms24
Copy link
Member

Lms24 commented Jun 22, 2022

Ahh I think I know what's going on here and I believe mocking getClient is fine: Because you're mocking getCurrentHub here to return the above created hub object, the getClient method was simply missing in the mock. The Transaction we access in _getBaggageWithSentryValues, when started/created, defaults to setting getCurrentHub as its hub which is what we end up using (or we directly call getCurrentHub if the transaction has no hub).

wondering if it's something that JS SDK has to fix

I don't think so (if my assumption above is correct). Can't really change this on the JS SDK though because we need the client at this stage to collect the information for the dynamic sampling context (i.e. the stuff we propagate via baggage and send in the envelope header).

@marandaneto
Copy link
Contributor Author

Thanks @Lms24 appreciate it.

@marandaneto marandaneto changed the title Bump Sentry JavaScript 7.2.0 Bump Sentry JavaScript 7.3.1 Jun 29, 2022
@marandaneto marandaneto marked this pull request as ready for review June 29, 2022 07:57
@marandaneto marandaneto enabled auto-merge (squash) June 29, 2022 08:00
@marandaneto marandaneto merged commit 64fb8a1 into main Jun 29, 2022
@marandaneto marandaneto deleted the bump/js720 branch June 29, 2022 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants