Skip to content

Conversation

@mydea
Copy link
Member

@mydea mydea commented Jun 28, 2024

This adds an E2E app using a custom OTEL setup with @opentelemetry/sdk-node.

It tests that data is sent both to sentry as well as to another OTLP exporter. For this, I adjusted the event proxy code to also allow to spin up a generic event proxy server (which I use for the OTLP exporter). I also rewrote this to use fetch as this is a bit easier to read IMHO.

This is a decent first step, we should add at least 2 more E2E test apps IMHO related to OTEL:

  1. An app using @opentelemetry/sdk-trace-node and some more custom tracing setup (e.g. more instrumentation, custom sampler, ....)
  2. An app using @opentelemetry/sdk-trace-node that does not use Sentry for performance at all, but only for errors, and only uses OTEL for trace monitoring.

Part of #12494

Comment on lines +189 to +193
return fetch(sentryIngestUrl, {
body: proxyRequestBody,
headers: reqHeaders,
method: proxyRequest.method,
}).then(async res => {

Check failure

Code scanning / CodeQL

Server-side request forgery

The [URL](1) of this request depends on a [user-provided value](2).
// eslint-disable-next-line no-console
console.log(
'[event-proxy-server] Warn: No dsn on envelope header. Maybe a client-report was received. Proxy request body:',
proxyRequestBody,

Check warning

Code scanning / CodeQL

Log injection

Log entry depends on a [user-provided value](1).
Copy link
Member

@chargome chargome left a comment

Choose a reason for hiding this comment

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

Nice! Good starting point for the remaining test setups I guess.

Copy link
Member

@s1gr1d s1gr1d left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for adding this test!
Would it make sense to also add one with .cjs?

Copy link
Member

@andreiborza andreiborza left a comment

Choose a reason for hiding this comment

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

nice!

@mydea
Copy link
Member Author

mydea commented Jun 28, 2024

Looks good, thanks for adding this test! Would it make sense to also add one with .cjs?

We can def. do this! Though this is transpiled to cjs, so it is at least run with cjs already :) Using ts helps to also test that types etc. match everywhere correctly :)

@mydea mydea force-pushed the fn/otel-e2e-app branch from 1f20dea to 1cfefd1 Compare July 1, 2024 07:47
@mydea mydea merged commit f4a289d into develop Jul 1, 2024
@mydea mydea deleted the fn/otel-e2e-app branch July 1, 2024 10:44
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.

5 participants