Fix: Sidecar URL issues fixed across spotlight #558

Shubhdeep12 · 2024-11-10T06:55:18Z

Before opening this PR:

I added a Changeset Entry with pnpm changeset:add
I referenced issues that this PR addresses

related #524

vercel · 2024-11-10T06:55:23Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
spotlightjs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 14, 2024 1:44pm

Shubhdeep12 · 2024-11-10T06:56:34Z

Didn't removed getting origin out of sidecar url because it can affect the users already passing sidecar stream url

packages/overlay/src/App.tsx

packages/overlay/src/components/OpenInEditor.tsx

BYK · 2024-11-11T13:31:43Z

packages/overlay/src/integrations/sentry/data/sentryDataCache.ts

@@ -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;


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

yeah lets use new URL()

packages/overlay/src/integrations/sentry/data/sentryDataCache.ts

BYK · 2024-11-11T13:33:44Z

packages/overlay/src/integrations/sentry/data/sentryDataCache.ts

    const { origin } = new URL(url);
+    this.sidecarUrl = origin;


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?

BYK · 2024-11-11T13:34:42Z

packages/overlay/src/sidecar.ts

+  const { origin } = new URL(sidecarUrl);
+  const sidecarStreamUrl: string = `${origin}/stream`;
+  const source = new EventSource(sidecarStreamUrl);


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

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?

BYK · 2024-11-11T13:36:05Z

packages/website/src/content/docs/sidecar/index.mdx

@@ -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",


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

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.

@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.

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.

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

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

BYK

I love where this patch is going, amazing!

Just added some notes and I think I spotted two errors. Once we resolve the errors, I'm happy to merge even without you addressing my other suggestions.

packages/overlay/src/App.tsx

packages/overlay/src/components/OpenInEditor.tsx

BYK · 2024-11-14T11:31:02Z

packages/overlay/src/components/OpenInEditor.tsx

 import { ReactComponent as PenIcon } from '../assets/pen.svg';

 export default function OpenInEditor({ file }: { file: string }) {
+  const { sidecarUrl } = useSpotlightContext();
+  const sidecarOpenUrl: string = new URL('/open', sidecarUrl).href;


Since you abstracted out sidecarUrl and this new URL(x, sidecarUrl).href looks like a pattern, do you think we can change the variable from the context to a function:

const { getSidecarUrl } = useSpotlightContext(); const openUrl = getSidecarUrl('open');

yeah we can do this

packages/overlay/src/index.tsx

packages/website/src/content/docs/reference/configuration.md

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and publish to npm yourself or [setup this action to publish automatically](https://github.com/changesets/action#with-publishing). If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @spotlightjs/[email protected] ### Minor Changes - Add request and module details to context with JSONViewer ([#544](#544)) - Add support for JSON-like queries (MongoDB) while improving span details page a bit ([#563](#563)) - - Added copy filename button on error frame. ([#550](#550)) - changed styling of error frames. - - Added subtabs in trace detail page ([#557](#557)) - - Sidecar url made generic to support all sidecar server routes. ([#558](#558)) - No use of static sidecar url. ### Patch Changes - - changed route and added navigation in performance tab to make queries tab default active. ([#555](#555)) - Fixed showing of 0 in false condition in span details. - Fixed minor overlay trigger count issue ([#542](#542)) - - Fixed some conditional rendering in TraceIcon and SpanDetails which was showing 0 on UI. ([#549](#549)) - Fixed routing for query summary page by encoding the query description which can be a long text. - Fix DB queries missing for some Sentry SDKs ([#547](#547)) ## @spotlightjs/[email protected] ### Minor Changes - - Sidecar url made generic to support all sidecar server routes. ([#558](#558)) - No use of static sidecar url. - Create a self-contained executable for Linux, macOS, and Windows for Spotlight. ([#559](#559)) Docker images now use these binaries instead of a Node build in the image. ### Patch Changes - Fix hanging when another Spotlight server is running ([#571](#571)) ## @spotlightjs/[email protected] ### Minor Changes - Add CLI banner for eye candy ([#568](#568)) - Add request and module details to context with JSONViewer ([#544](#544)) - Stabilize embedded JS file name for local includes ([#553](#553)) - - Sidecar url made generic to support all sidecar server routes. ([#558](#558)) - No use of static sidecar url. - Create a self-contained executable for Linux, macOS, and Windows for Spotlight. ([#559](#559)) Docker images now use these binaries instead of a Node build in the image. ### Patch Changes - Fix DB queries missing for some Sentry SDKs ([#547](#547)) - Fix hanging when another Spotlight server is running ([#571](#571)) - Updated dependencies \[[`4d1e07eda8e5ab04db2d1e6bb14aa823e2f5d4e0`](4d1e07e), [`3ec99001202fd66a81486cc49a47452eb9e3e34b`](3ec9900), [`2e4d90c369c6345b38c085cea89142647c4b6be2`](2e4d90c), [`84f029db0e76f9295b38349f659457878df6924d`](84f029d), [`ab5181c645bcee3ab42a4f28db4daab47b5852a5`](ab5181c), [`7b9329c8c59a25cfe4cec7cbd9c6f231cf099220`](7b9329c), [`6bd2937adfcd8721af6ccd5f031860691ce6e46f`](6bd2937), [`aba5c072c59f3777c47832d1532d732237b1b9dd`](aba5c07), [`1749cb3a06576a0d94f77f0831cd088045a762fb`](1749cb3), [`5c0b1624bac267554d93bc81d17414159f0b9fe0`](5c0b162), [`c88e50edd07c56c7368ff6788bbaa938e3f9f6b8`](c88e50e)]: - @spotlightjs/[email protected] - @spotlightjs/[email protected] ## @spotlightjs/[email protected] ### Patch Changes - Updated dependencies \[[`378b5c186742cb30949e5ba6342bd799b4c82b08`](378b5c1), [`2e4d90c369c6345b38c085cea89142647c4b6be2`](2e4d90c), [`ab5181c645bcee3ab42a4f28db4daab47b5852a5`](ab5181c), [`dad649a9f716d91be2665fd43f4d09497cde84c6`](dad649a), [`1749cb3a06576a0d94f77f0831cd088045a762fb`](1749cb3), [`5c0b1624bac267554d93bc81d17414159f0b9fe0`](5c0b162), [`c88e50edd07c56c7368ff6788bbaa938e3f9f6b8`](c88e50e)]: - @spotlightjs/[email protected] ## @spotlightjs/[email protected] ### Patch Changes - Updated dependencies \[[`4d1e07eda8e5ab04db2d1e6bb14aa823e2f5d4e0`](4d1e07e), [`3ec99001202fd66a81486cc49a47452eb9e3e34b`](3ec9900), [`2e4d90c369c6345b38c085cea89142647c4b6be2`](2e4d90c), [`84f029db0e76f9295b38349f659457878df6924d`](84f029d), [`ab5181c645bcee3ab42a4f28db4daab47b5852a5`](ab5181c), [`7b9329c8c59a25cfe4cec7cbd9c6f231cf099220`](7b9329c), [`6bd2937adfcd8721af6ccd5f031860691ce6e46f`](6bd2937), [`aba5c072c59f3777c47832d1532d732237b1b9dd`](aba5c07), [`1749cb3a06576a0d94f77f0831cd088045a762fb`](1749cb3), [`5c0b1624bac267554d93bc81d17414159f0b9fe0`](5c0b162), [`c88e50edd07c56c7368ff6788bbaa938e3f9f6b8`](c88e50e)]: - @spotlightjs/[email protected] - @spotlightjs/[email protected] Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

sidecar url fixed across spotlight

8cae97c

Shubhdeep12 requested a review from BYK November 10, 2024 06:55

vercel bot deployed to Preview November 10, 2024 06:55 View deployment

BYK requested changes Nov 11, 2024

View reviewed changes

fixed setting up sidecar url

856dd15

Shubhdeep12 requested a review from BYK November 13, 2024 05:02

vercel bot deployed to Preview November 13, 2024 05:03 View deployment

BYK requested changes Nov 14, 2024

View reviewed changes

some fixes after review comments.

278aa28

vercel bot deployed to Preview November 14, 2024 13:15 View deployment

sentry integration to not accept sidecarurl when its coming in context

9abb991

vercel bot deployed to Preview November 14, 2024 13:44 View deployment

Shubhdeep12 requested a review from BYK November 14, 2024 13:45

BYK enabled auto-merge (squash) November 14, 2024 14:00

BYK approved these changes Nov 14, 2024

View reviewed changes

BYK merged commit 1749cb3 into main Nov 14, 2024
11 checks passed

BYK deleted the shubh/fix/sidecar-url branch November 14, 2024 14:00

getsentry-release mentioned this pull request Nov 26, 2024

meta(changelog): Update package versions #573

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Sidecar URL issues fixed across spotlight #558

Fix: Sidecar URL issues fixed across spotlight #558

Shubhdeep12 commented Nov 10, 2024

vercel bot commented Nov 10, 2024 •

edited

Loading

Shubhdeep12 commented Nov 10, 2024

BYK Nov 11, 2024

Shubhdeep12 Nov 12, 2024

BYK Nov 11, 2024

BYK Nov 11, 2024

Shubhdeep12 Nov 13, 2024

BYK Nov 11, 2024

Shubhdeep12 Nov 12, 2024

BYK Nov 12, 2024

Shubhdeep12 Nov 13, 2024

Shubhdeep12 Nov 13, 2024 •

edited

Loading

Shubhdeep12 Nov 13, 2024

BYK left a comment

BYK Nov 14, 2024

Shubhdeep12 Nov 14, 2024

Fix: Sidecar URL issues fixed across spotlight #558

Fix: Sidecar URL issues fixed across spotlight #558

Conversation

Shubhdeep12 commented Nov 10, 2024

vercel bot commented Nov 10, 2024 • edited Loading

Shubhdeep12 commented Nov 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shubhdeep12 Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Nov 10, 2024 •

edited

Loading

Shubhdeep12 Nov 13, 2024 •

edited

Loading