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(node): Node client extends ServerRuntimeClient rather than BaseClient #8933

Merged

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Sep 1, 2023

Ref: #8693

Since common server bahaviour has now moved to ServerRuntimeClient, the Node client can now extend that rather than the base client.

This PR also moves the request session flusher to ServerRuntimeClient which leaves the Node client empty apart from a constructor wrapper to preserve backwards compatibilty.

@timfish
Copy link
Collaborator Author

timfish commented Sep 2, 2023

Still trying to work out how this could be impacting a couple of opentelemetry-node tests...

@AbhiPrasad
Copy link
Member

Any idea why the span processor doesn't work? 🤔

@timfish
Copy link
Collaborator Author

timfish commented Sep 12, 2023

Any idea why the span processor doesn't work? 🤔

I was going to ask you 😂

@AbhiPrasad
Copy link
Member

@timfish this commit fixes it e706b54

Had to just re-arrange the assertions, I guess the extra callback was making it jest resolve the test early?

@timfish
Copy link
Collaborator Author

timfish commented Sep 12, 2023

Ah wonderful. Thanks for working this out!

@timfish
Copy link
Collaborator Author

timfish commented Sep 13, 2023

The new test failures were all down to ServerRuntimeClient being derived from the EdgeClient code which used Promise.resolve for eventFromException and eventFromMessage.

These will become sync in v8 but for now they are using PromiseLike and in the NodeClient these have always used resolvedSyncPromise to keep everything synchronous. This PR was causing the event pipeline in the Node client to be async and changing the expected behaviour in a number of tests.

@@ -46,7 +49,7 @@ export class ServerRuntimeClient<
* @inheritDoc
*/
public eventFromException(exception: unknown, hint?: EventHint): PromiseLike<Event> {
return Promise.resolve(eventFromUnknownInput(getCurrentHub, this._options.stackParser, exception, hint));
return resolvedSyncPromise(eventFromUnknownInput(getCurrentHub, this._options.stackParser, exception, hint));
Copy link
Collaborator Author

@timfish timfish Sep 13, 2023

Choose a reason for hiding this comment

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

This cost me hours! 😭

Copy link
Member

Choose a reason for hiding this comment

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

Sorry you had to go through this! 😭

Thanks for figuring it out!!

@timfish timfish marked this pull request as ready for review September 13, 2023 22:48
@AbhiPrasad AbhiPrasad merged commit beec5be into getsentry:develop Sep 14, 2023
68 checks passed
@timfish timfish deleted the feat/node-use-ServerRuntimeClient branch September 14, 2023 08:40
@AbhiPrasad AbhiPrasad mentioned this pull request Sep 15, 2023
4 tasks
billyvg pushed a commit that referenced this pull request Sep 15, 2023
…seClient` (#8933)

Since common server bahaviour has now moved to `ServerRuntimeClient`,
the Node client can now extend that rather than the base client.

This PR also moves the request session flusher to `ServerRuntimeClient`
which leaves the Node client empty apart from a constructor wrapper to
preserve backwards compatibilty.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants