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.

Sign up for GitHub

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

Jump to bottom

Ensure Sentry ingestion does not block PR merges #11910

Closed
mydea opened this issue May 6, 2024 · 0 comments · Fixed by #12743
Closed

Ensure Sentry ingestion does not block PR merges #11910

mydea opened this issue May 6, 2024 · 0 comments · Fixed by #12743
Assignees
@mydea
Labels
Type: Tests

Comments

@mydea
Copy link
Member

mydea commented May 6, 2024

Today, most of our E2E tests ensure we send payloads to Sentry. This means we wait for stuff to be ingested by Sentry, which also means that if that is delayed or broken the tests failed, meaning we cannot merge PRs.

We do not want to be blocked like this. So to fix this, I propose that we:

  1. Create 1 or 2 new E2E tests that literally only test that we can send stuff to Sentry. We make this tests non-blocking, like the flaky test detector - meaning even if that fails you can merge a PR. There is no need to test this for every framework/SDK, it should be enough to maybe have one browser, one node, maybe next.js there.
  2. All other E2E tests stop testing that we send to sentry, and instead only check payloads etc. via the event proxy.
This was referenced May 28, 2024
Merged
Merged
mydea added a commit that referenced this issue May 28, 2024
@mydea
test: Improve E2E node tests (#12258)
d5f1caf 
This does two main things:

1. Updates node E2E tests to use `fetch` instead of axios
2. Update node E2E tests to avoid sending to Sentry.

Instead, we check everywhere via the proxy that the data sent is OK.


Part of #11910
mydea added a commit that referenced this issue May 29, 2024
@mydea
ci: Add optional E2E tests for sending to Sentry (#12259)
a1725a1 
This adds a new E2E test `react-send-to-sentry` that is run optionally.

For now this is the same as the old `standard-frontend-react` test -
eventually we can/should update that test (and others) to stop sending
to sentry.

This test will run in a separate group that we do not block merging on
when it fails.

For now, there is one browser and one node test that checks that they
send events successfully to Sentry - IMHO that should cover stuff
decently for now. I also made the source maps debug ID test optional, as
that inherently sends to Sentry.

We can in follow ups get rid of all the event sending stuff from the
remaining E2E tests.

Part of #11910
@mydea mydea self-assigned this May 29, 2024
This was referenced May 31, 2024
Merged
Merged
mydea added a commit that referenced this issue May 31, 2024
@mydea
test: Refactor webpack E2E tests to avoid sending to Sentry (#12312)
c68f1ad 
Part of #11910
@mydea mydea mentioned this issue May 31, 2024
Merged
mydea added a commit that referenced this issue May 31, 2024
@mydea
test(e2e): Streamline react-router-6-use-routes test (#12315)
31cb741 
Part of #11910 - we
already have error & transaction tests there, so we can just delete the
sending tests.
mydea added a commit that referenced this issue May 31, 2024
@mydea
test(e2e): Update react-router-6 test to avoid sending to Sentry (#…
70f9c14 
…12313)

Part of #11910

I renamed `standard-frontend-react` to `react-router-6` as this is more
descriptive (what does standard frontend even mean?).

This test now also uses the event proxy.
c298lee pushed a commit that referenced this issue Jun 4, 2024
@mydea @c298lee
test: Refactor webpack E2E tests to avoid sending to Sentry (#12312)
4772147 
Part of #11910
c298lee pushed a commit that referenced this issue Jun 4, 2024
@mydea @c298lee
test(e2e): Streamline react-router-6-use-routes test (#12315)
66a8cf9 
Part of #11910 - we
already have error & transaction tests there, so we can just delete the
sending tests.
c298lee pushed a commit that referenced this issue Jun 4, 2024
@mydea @c298lee
test(e2e): Update react-router-6 test to avoid sending to Sentry (#…
1880316 
…12313)

Part of #11910

I renamed `standard-frontend-react` to `react-router-6` as this is more
descriptive (what does standard frontend even mean?).

This test now also uses the event proxy.
@mydea mydea mentioned this issue Jun 5, 2024
Merged
mydea added a commit that referenced this issue Jun 6, 2024
@mydea
fix(node): Fix virtual parent span ID handling & update create-next-a…
0a6c1e5 
…pp E2E test (#12368)

This started out as updating the create-next-app test to not send to
sentry anymore, but instead check payloads.

However, while doing this, I noticed some inconsistencies, mostly that
there was a weird `parent_span_id` in api route transactions/errors
where there should be none.

**EDIT**

OK, another iteration on this!

Turns out setting an invalid spanID on a span will make OTEL ignore all
of this, including the trace ID, and instead will create a new trace ID
for a new span, which is not what we want. So we can't use this...

So instead, I now adjusted the already existing code to keep the
incoming parentSpanId on the trace state. The major change I did is
ensure we set this even if it is empty (it is set to an empty string
then). This way, we can identify if this has been set, and if it has,
use this as source of truth. And we can fall back to use the regular
parentSpanId if this is not set (for whatever reason).

**ORIGINAL**

So I set out to figure out what was happening there, and the problem was
that when continuing a virtual trace, we would construct a parent
spanContext like this:

```js
const spanContext: SpanContext = {
  traceId: propagationContext.traceId,
  spanId: propagationContext.parentSpanId || propagationContext.spanId,
  isRemote: true,
  traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE,
  traceState,
};
```

The problematic line is this: `spanId: propagationContext.parentSpanId
|| propagationContext.spanId,`. Since `spanId` is required on the
SpanContext, we had to set it to something, but
`propagationContext.parentSpanId` is by design often undefined. With
this behavior, we always set this to the random span ID we have on the
propagationContext, and picked this up downstream.

this now became:

```js
const spanContext: SpanContext = {
  traceId: propagationContext.traceId,
  spanId: propagationContext.parentSpanId || INVALID_SPANID,
  isRemote: true,
  traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE,
  traceState,
};
```

Plus a check further down:

```js
const traceState = makeTraceState({
  dsc,
  parentSpanId: spanId !== INVALID_SPANID ? spanId : undefined,
  sampled,
});
```

(Note, `INVALID_SPANID` is a constant exported from OTEL, which is
basically `0000....`).

I'll investigate in a follow up if it would make sense to always use
this for the propagation context, instead of a random one today, plus
ensuring that we always filter this out before we send, or something
like this 🤔

Part of #11910
@mydea mydea mentioned this issue Jun 7, 2024
Merged
mydea added a commit that referenced this issue Jun 10, 2024
@mydea
test(e2e): Update nextjs-app-dir E2E tests to avoid sending to Sentry (…
8b07998 
…#12403)

Part of #11910

This removes the tests in nextjs-app-dir where we still polled Sentry.
billyvg pushed a commit that referenced this issue Jun 10, 2024
@mydea @billyvg
fix(node): Fix virtual parent span ID handling & update create-next-a…
2d2c4bb 
…pp E2E test (#12368)

This started out as updating the create-next-app test to not send to
sentry anymore, but instead check payloads.

However, while doing this, I noticed some inconsistencies, mostly that
there was a weird `parent_span_id` in api route transactions/errors
where there should be none.

**EDIT**

OK, another iteration on this!

Turns out setting an invalid spanID on a span will make OTEL ignore all
of this, including the trace ID, and instead will create a new trace ID
for a new span, which is not what we want. So we can't use this...

So instead, I now adjusted the already existing code to keep the
incoming parentSpanId on the trace state. The major change I did is
ensure we set this even if it is empty (it is set to an empty string
then). This way, we can identify if this has been set, and if it has,
use this as source of truth. And we can fall back to use the regular
parentSpanId if this is not set (for whatever reason).

**ORIGINAL**

So I set out to figure out what was happening there, and the problem was
that when continuing a virtual trace, we would construct a parent
spanContext like this:

```js
const spanContext: SpanContext = {
  traceId: propagationContext.traceId,
  spanId: propagationContext.parentSpanId || propagationContext.spanId,
  isRemote: true,
  traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE,
  traceState,
};
```

The problematic line is this: `spanId: propagationContext.parentSpanId
|| propagationContext.spanId,`. Since `spanId` is required on the
SpanContext, we had to set it to something, but
`propagationContext.parentSpanId` is by design often undefined. With
this behavior, we always set this to the random span ID we have on the
propagationContext, and picked this up downstream.

this now became:

```js
const spanContext: SpanContext = {
  traceId: propagationContext.traceId,
  spanId: propagationContext.parentSpanId || INVALID_SPANID,
  isRemote: true,
  traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE,
  traceState,
};
```

Plus a check further down:

```js
const traceState = makeTraceState({
  dsc,
  parentSpanId: spanId !== INVALID_SPANID ? spanId : undefined,
  sampled,
});
```

(Note, `INVALID_SPANID` is a constant exported from OTEL, which is
basically `0000....`).

I'll investigate in a follow up if it would make sense to always use
this for the propagation context, instead of a random one today, plus
ensuring that we always filter this out before we send, or something
like this 🤔

Part of #11910
billyvg pushed a commit that referenced this issue Jun 10, 2024
@mydea @billyvg
test(e2e): Update nextjs-app-dir E2E tests to avoid sending to Sentry (…
7a36195 
…#12403)

Part of #11910

This removes the tests in nextjs-app-dir where we still polled Sentry.
mydea added a commit that referenced this issue Jul 4, 2024
@mydea
test(remix): Update remix E2E tests to avoid sending to Sentry
475f830 
Part of #11910
This was referenced Jul 4, 2024
Merged
Merged
mydea added a commit that referenced this issue Jul 4, 2024
@mydea
test(remix): Update remix E2E tests to avoid sending to Sentry
27f1f85 
Part of #11910
mydea added a commit that referenced this issue Jul 4, 2024
@mydea
test(remix): Update remix E2E tests to avoid sending to Sentry (#12750)
77ce28a 
Part of #11910

These are the last (non-optional) tests we have that still sent data to
Sentry! After that, E2E tests should be completely independent.

This also blocks
#12743
@mydea mydea closed this as completed in 8c0631a Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mydea mydea

Labels
Type: Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test(e2e): Do not require DSN to run E2E tests getsentry/sentry-javascript
2 participants
@mydea @AbhiPrasad