fix: Update JS Docs #1556

HazAT · 2020-03-18T10:19:30Z

No description provided.

src/collections/_documentation/performance/distributed-tracing.md

dashed · 2020-03-18T14:18:40Z

src/collections/_documentation/performance/distributed-tracing.md

    ],
+    tracesSampleRate: 0.1,


@HazAT Just curious, is this the preferred placement of the configuration? Wondering because of https://github.com/getsentry/sentry/blob/3672ca751d9306fc22761f1a52847787fcef7915/src/sentry/static/sentry/app/bootstrap.jsx#L37-L40

This will be fixed in the next update, should have never been there in the first place.

Gotcha. Thanks for clarifying 👍

src/collections/_documentation/performance/distributed-tracing.md

rhcarvalho · 2020-03-18T17:29:19Z

src/collections/_documentation/performance/distributed-tracing.md

@@ -280,7 +316,7 @@ const Apm = require("@sentry/apm"); // This is required since it patches functio

 Sentry.init({
    dsn: "___PUBLIC_DSN___",
-    tracesSampleRate: 0.1
+    tracesSampleRate: 1


Why change it to 1?

Daniel, we cannot allow anybody to send 100% of their traffic to us. This burns immediately through their quota as long as the new billing is not live.

I leave it in the responsibility of the user to set the correct value here.
1 = they see it works
0.1 = they write into support it's not working

Okay, can we then add a comment here that they should be careful in increasing this towards 1 in prod? 1 makes total sense in dev.

FWIW this is also inconsistent with other code examples.

can we then add a comment here that they should be careful in increasing this towards 1 in prod? 1 makes total sense in dev.

+1

Maybe nobody cares, but I would also find 1.0 more indicative that this is not a 0 or 1 setting, but that typical values are floats in the [0.0, 1.0] range: 0.0, 0.1, 0.25...

@HazAT
maybe keep it at 1.0 but then put a comment before or after the code snippet that says,
"We recommend 0.1 for when you put this to production [because...]"

maybe keep it at 0.1 but then put a comment before or after the code snippet that says,
"You may want this on 1.0 if you're testing it for the first time"
"You may want this on 1.0 if you're testing it for the first time or else you might think your transactions aren't coming through"

you're 100% correct about them writing into support, as I tricked myself on this the first time I set it up, and then again a month later when I re-visisted it. So that makes me prefer to see it at 1.0 by default.

Co-Authored-By: Rodolfo Carvalho <[email protected]>

…cs into performance/sampling

rhcarvalho

Thanks for adding this! A few suggestions.

rhcarvalho · 2020-03-20T09:43:05Z

src/collections/_documentation/performance/distributed-tracing.md

+- Your frontend is running `mywebapp.com`
+- Your backend is running `api.mybackend.com`
+- You setup your frontend to make API calls to `api.mybackend.com`
+- Then the option need to be configured like this `new ApmIntegrations.Tracing({tracingOrigins: ['localhost', /^\//, 'api.mybackend.com']})`


What's the /^\// for in this case?
Also, I'd not include localhost, because what's the point of sending the header to localhost (the page visitor's loopback device). Users may need to arrange for this config to have different values depending on environment.

I would suggest only adding a new bullet point before this one without changing the code sample:

- In development, your API is at `localhost`

To explain what the localhost is needed for. Then they can make an informed choice on which values to include in their config

The choice of hosts is very env specific. If you're developing on local containers, localhost is probably the wrong value. We can't know what the right value is for everyone, but we can explain what it needs to match and how to detect errors/ fix it.

@rhcarvalho I think
it's for checking that the string 'localhost' is validated or not by the regex /^// ?

check out the function isMatchingPattern in string.ts

// tracing.ts const isWhitelisted = self._options.tracingOrigins.some((origin: string | RegExp) => isMatchingPattern(url, origin), );

looks like you can also write ['localhost', 'string']?

// string.ts export function isMatchingPattern(value: string, pattern: RegExp | string): boolean { if (isRegExp(pattern)) { return (pattern as RegExp).test(value); } if (typeof pattern === 'string') { return value.includes(pattern); } return false; }

but i'm a little confused because:

reg = /^\// reg.test('foobar') false reg.test('foo/bar') false

@rhcarvalho nevermind, the regex is checking that the string doesn't begin with a forward slash.

src/collections/_documentation/performance/distributed-tracing.md

Co-Authored-By: Rodolfo Carvalho <[email protected]>

rhcarvalho · 2020-03-20T19:30:57Z

src/collections/_documentation/performance/distributed-tracing.md

 // When we pop the activity, the Integration will finish the span and after the timeout finish the transaction and send it to Sentry
-Integrations.ApmIntegrations.popActivity(activity);
+// Keep in mind, as long as you do not pop the activity, the transaction will be kept alive and not sent to Sentry


We ended up with two sentences that perhaps could be combined together to better convey the important information? ("When we pop... send it to Sentry" & "Keep in mind, ... sent to Sentry")

src/collections/_documentation/performance/distributed-tracing.md

rhcarvalho · 2020-03-20T19:33:30Z

src/collections/_documentation/performance/distributed-tracing.md

+// Let's say this function is invoked when a user clicks on the checkout button of your shop
+shopCheckout() {
+    // This will create a new Transaction for you
+    ApmIntegrations.Tracing.startIdleTransaction('shopCheckout');


Why here we use ApmIntegrations.Tracing.startIdleTransaction instead of hub.startSpan?

Because we prefer ppl using the Integration over the startSpan
The integration also adds spans for XHR/fetch

src/collections/_documentation/performance/distributed-tracing.md

rhcarvalho · 2020-03-20T19:37:52Z

src/collections/_documentation/performance/distributed-tracing.md

+    const activity = ApmIntegrations.Tracing.pushActivity('task', {
+        data: {
+            result
+        },
+        op: 'task',


Why do we need a name (first arg) for an Activity? Why would people need to write an activity name and then op (which is also some sort of activity/operation name)?!

It's there if you want to have an activity that is "blocking" the idle transaction but you do not want to create a span out of it.

Co-Authored-By: Rodolfo Carvalho <[email protected]>

dashed

Looks good to me 👍

fix: Sampling rate

3821bf4

HazAT self-assigned this Mar 18, 2020

HazAT mentioned this pull request Mar 18, 2020

fix(apm): Sampling of traces getsentry/sentry-javascript#2500

Merged

ref: Descript activity and add another JS example

592c2d5

HazAT changed the title ~~fix: Sampling rate~~ fix: Update JS Docs Mar 18, 2020

HazAT requested review from dashed and MimiDumpling and removed request for dashed March 18, 2020 13:25

rhcarvalho reviewed Mar 18, 2020

View reviewed changes

fix: Typo

8352af9

dashed reviewed Mar 18, 2020

View reviewed changes

dashed requested a review from a team March 18, 2020 14:19

rhcarvalho reviewed Mar 18, 2020

View reviewed changes

src/collections/_documentation/performance/distributed-tracing.md Outdated Show resolved Hide resolved

rhcarvalho reviewed Mar 18, 2020

View reviewed changes

HazAT and others added 2 commits March 18, 2020 19:08

Update src/collections/_documentation/performance/distributed-tracing.md

7755045

Co-Authored-By: Rodolfo Carvalho <[email protected]>

Update src/collections/_documentation/performance/distributed-tracing.md

bb50532

Co-Authored-By: Rodolfo Carvalho <[email protected]>

MimiDumpling requested a review from PeloWriter March 19, 2020 00:06

HazAT added 2 commits March 20, 2020 10:16

feat: Explain tracingOrigins and CORS

4fb9cba

Merge branch 'performance/sampling' of github.com:getsentry/sentry-do…

c00a66f

…cs into performance/sampling

rhcarvalho reviewed Mar 20, 2020

View reviewed changes

dashed self-requested a review March 20, 2020 10:28

Apply suggestions from code review

d5d27a0

Co-Authored-By: Rodolfo Carvalho <[email protected]>

rhcarvalho reviewed Mar 20, 2020

View reviewed changes

HazAT and others added 2 commits March 21, 2020 14:44

Update src/collections/_documentation/performance/distributed-tracing.md

d2bc4a8

Co-Authored-By: Rodolfo Carvalho <[email protected]>

fix: CodeReview

6e2975d

dashed approved these changes Mar 23, 2020

View reviewed changes

HazAT merged commit 8b6aa07 into master Mar 23, 2020

HazAT deleted the performance/sampling branch March 23, 2020 08:37

github-actions bot locked and limited conversation to collaborators Dec 16, 2020

Uh oh!

fix: Update JS Docs #1556

fix: Update JS Docs #1556

Uh oh!

Conversation

HazAT commented Mar 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhcarvalho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

untitaker Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rhcarvalho Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dashed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

untitaker Mar 20, 2020 •

edited

Loading

rhcarvalho Mar 20, 2020 •

edited

Loading