Draft: Upgrading from raven-js to sentry/browser #2417

BenzeneAlcohol · 2024-07-19T10:32:05Z

Which problem is this PR solving?

Resolves #2412

Description of the changes

I've migrated a part of the things, but I want to know whether the approach is correct or it needs to be different. Hence a draft MR. The names of files, variables etc haven't been changed, this is just in progress.

How was this change tested?

Test cases for now, the old ones with the same payload.

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Signed-off-by: Muthukumar <[email protected]>

packages/jaeger-ui/src/utils/tracking/conv-raven-to-ga.tsx

package.json

BenzeneAlcohol · 2024-07-21T15:04:46Z

If you see packages/jaeger-ui/src/utils/tracking/ga.tsx, autoBreadcrumbs there are configured to accept location as well to be recorded for eveyr event. However , from sentry's docs for breadcrumbs, it seems like the location param is absent. What do we do we here?

https://docs.sentry.io/platforms/javascript/configuration/integrations/breadcrumbs/

Migration in progress Signed-off-by: Muthukumar <[email protected]>

Signed-off-by: Muthukumar <[email protected]>

codecov · 2024-07-23T10:32:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.61%. Comparing base (322fd98) to head (7b1ad96).
Report is 16 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2417      +/-   ##
==========================================
+ Coverage   96.60%   96.61%   +0.01%     
==========================================
  Files         254      254              
  Lines        7662     7688      +26     
  Branches     1996     1993       -3     
==========================================
+ Hits         7402     7428      +26     
  Misses        260      260

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Muthukumar <[email protected]>

BenzeneAlcohol · 2024-07-23T10:43:05Z

The PR is almost over, I need to change file names and function names. For now the existing unit tests pass, but any other ways to test this properly? @yurishkuro

yurishkuro · 2024-07-23T19:36:44Z

You can set up your own property in GA and test that Jaeger UI generates the desired events, as well as compare them with events generated by the UI before your change. You will probably need to induce some exceptions, e.g. by using Upload function in Search tab and uploading a malformed text.

BenzeneAlcohol · 2024-07-23T19:50:23Z

You can set up your own property in GA and test that Jaeger UI generates the desired events, as well as compare them with events generated by the UI before your change. You will probably need to induce some exceptions, e.g. by using Upload function in Search tab and uploading a malformed text.

Alright, I'll try that. Meanwhile, can you review the code and see if there are any obvious mistakes?

packages/jaeger-ui/package.json

packages/jaeger-ui/src/types/tracking.tsx

yurishkuro · 2024-07-23T20:10:09Z

packages/jaeger-ui/src/utils/tracking/conv-raven-to-ga.tsx

@@ -169,9 +160,7 @@ function convNav(to: string) {
 //    "dp" - fetch the dependency data
 // And, "NNN" is a non-200 status code.
 type TCrumbData = {


can you explain what "breadcrumb" means in this context? I saw a link you posted to sentry docs, but they didn't explain it either, just started talking details.

Will explain this tomorrow, late night here.

breadcrumb in this context is basically any event that is captured. Everything is referred to as a breadcrumb. It basically lets us capture everything leading up to that event/error.

packages/jaeger-ui/src/utils/tracking/ga.tsx

Signed-off-by: Muthukumar <[email protected]>

README has to be updated, removing the old raven-sdk information. Signed-off-by: Muthukumar <[email protected]>

yurishkuro

will you be able to test against Google Analytics?

yurishkuro · 2024-07-24T23:04:22Z

packages/jaeger-ui/src/types/tracking.tsx

@@ -22,7 +22,7 @@ export interface IWebAnalyticsFunc {

 export default interface IWebAnalytics {
  init: () => void;
-  context: boolean | RavenStatic | null;
+  context: boolean | typeof BrowserClient | null;


This field is rather confusing to me. Is it a readable property for the user of IWebAnalytics interface? Or is it an input to the implementation of the interface? If it's the latter, why does it need to be exposed in the interface?

Not specifically related to the PR, but I am trying to understand the structure of the code.

So here, IWebAnalytics is an interface of the object that is returned by the function IWebAnalyticsFunc that takes in 3 variables as the input.

This is used in ga.tsx as function GA. There, if you notice, context is assigned to BrowserClient, and not an instance of it. That is why I have kept context as either a boolean, null, or a type of BrowserClient object, and not an instance of it.

Even I am a little confused as to the places it being used.

It is set depending on whether error tracking is enabled on not. It needs to be there on the interface because depending on the config, this needs to be set.

It is used in L56-57 in packages/jaeger-ui/src/utils/tracking/index.tsx

packages/jaeger-ui/src/utils/tracking/fixtures.js

yurishkuro · 2024-07-24T23:09:31Z

packages/jaeger-ui/src/utils/tracking/conv-sentry-to-ga.tsx

@@ -247,20 +236,14 @@ function compressCssSelector(selector: string) {
 // The chronological ordering of the breadcrumbs is older events precede newer
 // events. This ordering was kept because it's easier to see which page events
 // occurred on.
-interface ICrumb {


it seems like we used an abstraction over Raven type (even though abstraction itself is a bit coupled, but an abstraction nonetheless, i.e. could be implemented by something other than Raven). What's the reason to remove it instead of just adapting? Adapting seems to only require optionality on the fields, which would minimize the changes throughout this file.

This does make sense. But why I thought of using the Breadcrumb interface from Sentry was because this function convSentryToGa is called from trackSentryError in L192, packages/jaeger-ui/src/utils/tracking/ga.tsx.

So there, the transport options come from the Event interface in Sentry, which has a breadcrumb field which uses the Breadcrumb interface. I just thought of keeping it uniform everywhere and since that field already uses the same interface, used it here as well.

BenzeneAlcohol · 2024-07-25T06:12:06Z

will you be able to test against Google Analytics?

Could be better if someone who already has an idea about GA does it. Can you do it? Else I can go through the docs and setup, but will need one or two days.

Signed-off-by: Muthukumar <[email protected]>

yurishkuro · 2024-07-29T00:50:33Z

Could be better if someone who already has an idea about GA does it. Can you do it? Else I can go through the docs and setup, but will need one or two days.

I don't have any of that set up either. At minimum you can enable debug mode and see what the tracking logs into console and compare it with the before version.

BenzeneAlcohol · 2024-07-29T06:15:47Z

i'll do.

BenzeneAlcohol · 2024-07-31T15:19:06Z

Hey, I got caught up with a bit of work stuff. I'll test this PR tomorrow and day after, and hopefully by this weekend we can get this done. @yurishkuro

BenzeneAlcohol · 2024-08-07T05:52:31Z

I tried, but couldn't really find a way to troubleshoot using just the console.log statements. I went through a bunch of tests that use this config in a lot of places, so since they have passed, I am 90% sure this works. It is just the other 10% I'm a little concerned about.

yurishkuro · 2024-08-08T01:18:56Z

Is it possible to test with a real GA account / property? There's also a Chrome extension that claims to help with testing.

BenzeneAlcohol · 2024-08-09T08:46:23Z

Is it possible to test with a real GA account / property? There's also a Chrome extension that claims to help with testing.

Will checkt that out

Just testing and debugging

4692898

Signed-off-by: Muthukumar <[email protected]>

BenzeneAlcohol requested a review from a team as a code owner July 19, 2024 10:32

BenzeneAlcohol requested review from pavolloffay and removed request for a team July 19, 2024 10:32

BenzeneAlcohol marked this pull request as draft July 19, 2024 10:32

yurishkuro reviewed Jul 19, 2024

View reviewed changes

packages/jaeger-ui/src/utils/tracking/conv-raven-to-ga.tsx Outdated Show resolved Hide resolved

anshgoyalevil reviewed Jul 20, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

BenzeneAlcohol added 2 commits July 23, 2024 15:45

Resolves jaegertracing#2412

9423f5e

Migration in progress Signed-off-by: Muthukumar <[email protected]>

Removed and re added package

8465d93

Signed-off-by: Muthukumar <[email protected]>

Linting fixes

7b1ad96

Signed-off-by: Muthukumar <[email protected]>

BenzeneAlcohol marked this pull request as ready for review July 23, 2024 10:43

yurishkuro added the changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements label Jul 23, 2024

yurishkuro reviewed Jul 23, 2024

View reviewed changes

BenzeneAlcohol added 3 commits July 24, 2024 15:17

Yarn lock committed

c06f369

Signed-off-by: Muthukumar <[email protected]>

Imported only the necessary modules

78f88eb

Signed-off-by: Muthukumar <[email protected]>

Changed file names from raven

f1bfa93

README has to be updated, removing the old raven-sdk information. Signed-off-by: Muthukumar <[email protected]>

yurishkuro reviewed Jul 24, 2024

View reviewed changes

Fix typo

b58795f

Signed-off-by: Muthukumar <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Upgrading from raven-js to sentry/browser #2417

Draft: Upgrading from raven-js to sentry/browser #2417

BenzeneAlcohol commented Jul 19, 2024 •

edited

Loading

BenzeneAlcohol commented Jul 21, 2024

codecov bot commented Jul 23, 2024 •

edited

Loading

BenzeneAlcohol commented Jul 23, 2024

yurishkuro commented Jul 23, 2024

BenzeneAlcohol commented Jul 23, 2024

yurishkuro Jul 23, 2024

BenzeneAlcohol Jul 23, 2024

BenzeneAlcohol Jul 24, 2024

yurishkuro left a comment

yurishkuro Jul 24, 2024

BenzeneAlcohol Jul 25, 2024

BenzeneAlcohol Jul 25, 2024

yurishkuro Jul 24, 2024

BenzeneAlcohol Jul 25, 2024

BenzeneAlcohol commented Jul 25, 2024

yurishkuro commented Jul 29, 2024

BenzeneAlcohol commented Jul 29, 2024

BenzeneAlcohol commented Jul 31, 2024

BenzeneAlcohol commented Aug 7, 2024

yurishkuro commented Aug 8, 2024

BenzeneAlcohol commented Aug 9, 2024

Draft: Upgrading from raven-js to sentry/browser #2417

Are you sure you want to change the base?

Draft: Upgrading from raven-js to sentry/browser #2417

Conversation

BenzeneAlcohol commented Jul 19, 2024 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

BenzeneAlcohol commented Jul 21, 2024

codecov bot commented Jul 23, 2024 • edited Loading

Codecov Report

BenzeneAlcohol commented Jul 23, 2024

yurishkuro commented Jul 23, 2024

BenzeneAlcohol commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a 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

BenzeneAlcohol commented Jul 25, 2024

yurishkuro commented Jul 29, 2024

BenzeneAlcohol commented Jul 29, 2024

BenzeneAlcohol commented Jul 31, 2024

BenzeneAlcohol commented Aug 7, 2024

yurishkuro commented Aug 8, 2024

BenzeneAlcohol commented Aug 9, 2024

BenzeneAlcohol commented Jul 19, 2024 •

edited

Loading

codecov bot commented Jul 23, 2024 •

edited

Loading