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
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
Next Next commit
sidecar url fixed across spotlight
  • Loading branch information
Shubhdeep12 committed Nov 10, 2024
commit 8cae97c9dfcecb872149d045f8c4b6dc6822f8a9
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
@@ -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',
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',
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,
Original file line number Diff line number Diff line change
@@ -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 & {
@@ -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',
4 changes: 3 additions & 1 deletion packages/overlay/src/sidecar.ts
Original file line number Diff line number Diff line change
@@ -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);
2 changes: 1 addition & 1 deletion packages/overlay/src/types.ts
Original file line number Diff line number Diff line change
@@ -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;

8 changes: 5 additions & 3 deletions packages/spotlight/src/vite-plugin.ts
Original file line number Diff line number Diff line change
@@ -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,
@@ -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;
@@ -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' },
@@ -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' },
6 changes: 3 additions & 3 deletions packages/website/src/content/docs/reference/configuration.md
Original file line number Diff line number Diff line change
@@ -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',
});
```

2 changes: 1 addition & 1 deletion packages/website/src/content/docs/sidecar/index.mdx
Original file line number Diff line number Diff line change
@@ -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.

)
```

Loading