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

Fix: Sidecar URL issues fixed across spotlight #558

Merged
merged 4 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changeset/twenty-fireants-fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@spotlightjs/spotlight': minor
'@spotlightjs/overlay': minor
'@spotlightjs/sidecar': minor
---

- Sidecar url made generic to support all sidecar server routes.
- No use of static sidecar url.
6 changes: 3 additions & 3 deletions packages/overlay/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,10 @@ export default function App({
// See https://github.com/remix-run/react-router/issues/7634
const navigate = useNavigate();
const clearEvents = useCallback(async () => {
const { origin } = new URL(sidecarUrl);
const clearEventsUrl: string = `${origin}/clear`;

try {
const { origin } = new URL(sidecarUrl);
const clearEventsUrl: string = `${origin}/clear`;

await db.reset();
await fetch(clearEventsUrl, {
method: 'DELETE',
Expand Down
4 changes: 2 additions & 2 deletions packages/overlay/src/components/OpenInEditor.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import type React from 'react';
import { useCallback } from 'react';
import sentryDataCache from '~/integrations/sentry/data/sentryDataCache';
import { ReactComponent as PenIcon } from '../assets/pen.svg';

export default function OpenInEditor({ file }: { file: string }) {
const openInEditor = useCallback(
(evt: React.MouseEvent) => {
// TODO: Make this URL dynamic based on sidecarUrl!
fetch('http://localhost:8969/open', {
fetch(`${sentryDataCache.getSidecarUrl()}/open`, {
method: 'POST',
body: file,
credentials: 'omit',
Expand Down
2 changes: 1 addition & 1 deletion packages/overlay/src/constants.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export const DEFAULT_SIDECAR_URL = 'http://localhost:8969/stream';
export const DEFAULT_SIDECAR_URL = 'http://localhost:8969';

export const DEFAULT_EXPERIMENTS = {
'sentry:focus-local-events': true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ class SentryDataCache {

protected subscribers: Map<string, Subscription> = new Map();

protected contextLinesProvider: string = new URL(DEFAULT_SIDECAR_URL).origin + CONTEXT_LINES_ENDPOINT;
protected sidecarUrl: string = new URL(DEFAULT_SIDECAR_URL).origin;
protected contextLinesProvider: string = this.sidecarUrl + CONTEXT_LINES_ENDPOINT;
Copy link
Member

Choose a reason for hiding this comment

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

Speaking of hypocrisy... 😅 -- Same comment above, we should probably use some sort of a URL builder at this point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah lets use new URL()


constructor(
initial: (SentryEvent & {
Expand All @@ -46,9 +47,14 @@ class SentryDataCache {

setSidecarUrl(url: string) {
const { origin } = new URL(url);
this.sidecarUrl = origin;
Copy link
Member

Choose a reason for hiding this comment

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

If we go back to the url.origin we are not solving the problem reported in #524 as they want to be able to use the sidecar under a main root URL. That means we should let go of using .origin and slice the path up and just strip the last /stream to get the "root" sidecar URL. Makes sense?

this.contextLinesProvider = origin + CONTEXT_LINES_ENDPOINT;
}

getSidecarUrl() {
return this.sidecarUrl;
}

inferSdkFromEvent(event: SentryEvent) {
const sdk: Sdk = {
name: 'unknown',
Expand Down
4 changes: 3 additions & 1 deletion packages/overlay/src/sidecar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ export function connectToSidecar(
setOnline: React.Dispatch<React.SetStateAction<boolean>>,
): () => void {
log('Connecting to sidecar at', sidecarUrl);
const source = new EventSource(sidecarUrl);
const { origin } = new URL(sidecarUrl);
const sidecarStreamUrl: string = `${origin}/stream`;
const source = new EventSource(sidecarStreamUrl);
Copy link
Member

Choose a reason for hiding this comment

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

Feels like this should be the ground truth for the sidecarURL and we should not have anything stored in sentryDataCache regarding the URL?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey @BYK, could you please explain what you meant here?

The sidecarURL is either provided by the user or defaults to a pre-set URL, and then it's passed here in this file. Ideally, to use the sidecarURL effectively, we should store it somewhere.

Are you suggesting that instead of storing it in SentryDataCache, we should store it somewhere else, like in a context, so that other integrations can also access the sidecarURL?


for (const [contentType, listener] of Object.entries(contentTypeListeners)) {
source.addEventListener(contentType, listener);
Expand Down
2 changes: 1 addition & 1 deletion packages/overlay/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export type SpotlightOverlayOptions = {
* will use this URL instead of the default URL to connect to the sidecar
* and to listen to incoming events.
*
* @default "http://localhost:8969/stream"
* @default "http://localhost:8969"
*/
sidecarUrl?: string;

Expand Down
8 changes: 5 additions & 3 deletions packages/spotlight/src/vite-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ const serverOptions = new Set(['importPath', 'integrationNames', 'port']);

export function buildClientInit(options: SpotlightInitOptions) {
const clientOptions = Object.fromEntries(
Object.entries(options).filter(([key, _value]) => !key.startsWith('_') && !serverOptions.has(key)),
Object.entries(options).filter(([key]) => !key.startsWith('_') && !serverOptions.has(key)),
);
let initOptions = JSON.stringify({
...clientOptions,
Expand All @@ -61,7 +61,7 @@ export function buildClientInit(options: SpotlightInitOptions) {
].join('\n');
}

async function sendErrorToSpotlight(err: ErrorPayload['err'], spotlightUrl: string = 'http://localhost:8969/stream') {
async function sendErrorToSpotlight(err: ErrorPayload['err'], spotlightUrl: string = 'http://localhost:8969') {
if (!err.errors) {
console.log(err);
return;
Expand All @@ -72,6 +72,8 @@ async function sendErrorToSpotlight(err: ErrorPayload['err'], spotlightUrl: stri
const errorLineInContext = contextLines?.indexOf(errorLine);
const event_id = randomBytes(16).toString('hex');
const timestamp = new Date();
const { origin } = new URL(spotlightUrl);
const spotlightStreamUrl: string = `${origin}/stream`;
const envelope = [
{ event_id, sent_at: timestamp.toISOString() },
{ type: 'event' },
Expand Down Expand Up @@ -117,7 +119,7 @@ async function sendErrorToSpotlight(err: ErrorPayload['err'], spotlightUrl: stri
]
.map(p => JSON.stringify(p))
.join('\n');
return await fetch(spotlightUrl, {
return await fetch(spotlightStreamUrl, {
method: 'POST',
body: envelope,
headers: { 'Content-Type': 'application/x-sentry-envelope' },
Expand Down
6 changes: 3 additions & 3 deletions packages/website/src/content/docs/reference/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,15 @@ init({

#### `sidecarUrl`

**type:** `string` **default:** `"http://localhost:8969/stream"`
**type:** `string` **default:** `"http://localhost:8969"`

The Sidecar event-source stream endpoint URL.
The Sidecar event-source endpoint URL.

Set this option if you have the sidecar running on another URL than the default one.

```ts
init({
sidecarUrl: 'http://localhost:8969/stream',
sidecarUrl: 'http://localhost:8969',
});
```

Expand Down
2 changes: 1 addition & 1 deletion packages/website/src/content/docs/sidecar/index.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ Also in the Sentry SDKs the URL can be configured via the `spotlight` option, fo
import sentry_sdk

sentry_sdk.init(
spotlight="http://localhost:8969/stream",
spotlight="http://localhost:8969",
Copy link
Member

Choose a reason for hiding this comment

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

No no no definitely not. The SDKs expect the /stream part and at this point we should keep it that way unfortunately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, but ideally we should just ask for sidecar url.
I know initially, we were only having stream url i.e. why i guess we asked for stream url directly.

let me think if we could do something to support /stream url as well here.

Copy link
Member

Choose a reason for hiding this comment

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

@Shubhdeep12 I think a cheap hack is to see if we have the URL ending with /stream and try to guess from there.

That said not having the /stream part in the SDKs is a no go, as it requires us to change all existing SDKs to support this and have a fallback or cut a major release. Don't want either so that ship seems to have sailed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I wasn't intending to remove the /stream path. I meant that the user should focus on just sending the sidecarURL while initializing Spotlight. We'll handle which path to use for different cases, like:

  • /stream for EventSource,
  • /clear for clearing all events,
  • /open for opening the editor, etc.

Copy link
Collaborator Author

@Shubhdeep12 Shubhdeep12 Nov 13, 2024

Choose a reason for hiding this comment

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

Can we do this-

  • Remove the /stream suffix(if present) from sidecar URL passed in Spotlight.init
  • Store the sidecar url
  • and then using /stream, /open, /clear ,etc based on the need in the code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just pushed this, let me know your thoughts on this.

)
```

Expand Down
Loading