Skip to content

Conversation

Zen-cronic
Copy link
Contributor

@Zen-cronic Zen-cronic commented Sep 3, 2024

Resolves #13310

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

@Zen-cronic
Copy link
Contributor Author

Zen-cronic commented Sep 5, 2024

Tests to be added. WIP

@Zen-cronic Zen-cronic marked this pull request as ready for review September 14, 2024 01:14

createRunner(__dirname, 'scenario.js')
.expect({ transaction: SERVER_TRANSACTION })
.expect({ transaction: CLIENT_TRANSACTION })
Copy link
Contributor Author

@Zen-cronic Zen-cronic Sep 14, 2024

Choose a reason for hiding this comment

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

I'm not sure why the server socket and client socket are not part of the same transaction. In scenario.js, the server socket emits an event to the client, which triggers the client to send back to the server.

Update: the transactions are also not part of the same trace (different trace_id) - verified by debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this is expected given the same behaviour in other integrations where consumer and producer are separate.

@Zen-cronic Zen-cronic closed this Sep 14, 2024
@Zen-cronic Zen-cronic force-pushed the feat/socketioIntegration-node branch from 93e0e53 to 8d2e189 Compare September 14, 2024 01:24
Add otel integration and e2e tests.

Signed-off-by: Kaung Zin Hein <[email protected]>
@Zen-cronic Zen-cronic reopened this Sep 14, 2024
onHook(span) {
addOriginToSpan(span, 'auto.socket.otel.consumer');
},
traceReserved: true,
Copy link
Contributor Author

@Zen-cronic Zen-cronic Sep 15, 2024

Choose a reason for hiding this comment

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

We should expose options such as traceReserved, emitIgnoreEventList, and onIgnoreEventList for users to configure.

Choose a reason for hiding this comment

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

I agree but for now this looks like a great start!

@Zen-cronic
Copy link
Contributor Author

i'm not sure why but the test setup is flaky - failing in some node versions in the ci, and locally failing sometimes.

@AbhiPrasad
Copy link
Member

Closing because of #13310 (comment)

@AbhiPrasad AbhiPrasad closed this Nov 11, 2024
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.

Add socketIoIntegration to Node

3 participants