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

feat: APM updates #2161

Merged
merged 67 commits into from
Dec 2, 2019
Merged

feat: APM updates #2161

merged 67 commits into from
Dec 2, 2019

Conversation

kamilogorek
Copy link
Contributor

@kamilogorek kamilogorek commented Jul 10, 2019

This PR provides the new @sentry/apm package with a new Tracing integration.
The Tracing integration creates Transactions for pageloads as well as navigation changes. Additionally, it will add the sentry-trace header so if used together with an apm enabled server SDK of Sentry (Node, Python) you will get the full trace.

We also hook into global XHR & fetch calls and create spans out of it that will be attached to the transaction.

@HazAT HazAT changed the title wip: Session Tracking feat: APM updates Jul 17, 2019
@getsentry-bot
Copy link
Contributor

getsentry-bot commented Jul 18, 2019

Messages
📖 ✅ TSLint passed
📖

@sentry/browser bundle gzip'ed minified size: (ES5: 16.5586 kB) (ES6: 15.5605 kB)

Generated by 🚫 dangerJS against 073920f

@HazAT HazAT changed the title feat: APM updates [WIP] feat: APM updates Jul 19, 2019
packages/hub/src/span.ts Outdated Show resolved Hide resolved
@kamilogorek
Copy link
Contributor Author

cc @mitsuhiko for last commit

const span = top.scope.getSpan();

if (span) {
return span.newSpan(spanContext);
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this childSpan instead of newSpan

packages/hub/src/span.ts Outdated Show resolved Hide resolved
@ajacques
Copy link

What's the long-term plan around APM in Sentry? Is there overlap with or a plan to integrate with OpenCensus/OpenTelemetry? OpenCensus just released a prototypeJavaScript implementation and I'd love to avoid having to integrate 2-3 different tracing systems.

@kamilogorek
Copy link
Contributor Author

@ajacques we're still in early PoC phase so there's not much I can say for certain, but we do have OC/OT in mind while working on it :)

@HazAT
Copy link
Member

HazAT commented Oct 4, 2019

TODO: (!) Circular dependency: ../hub/src/hub.ts -> ../hub/src/span.ts -> ../hub/src/hub.ts

@Bessonov
Copy link

Hi guys, I found the blog post about Sentry APM. As I understand, Sentry wants to support not only error logging, but also tracing, right? Is there any plan to support "normal" Logging for better analysability of errors or do we still need tools like ELK?

packages/integrations/package.json Outdated Show resolved Hide resolved
packages/integrations/src/express.ts Outdated Show resolved Hide resolved
packages/integrations/src/express.ts Outdated Show resolved Hide resolved
packages/integrations/src/express.ts Outdated Show resolved Hide resolved
@@ -93,6 +95,10 @@ export class TransactionActivity implements Integration {
if (TransactionActivity._enabled !== undefined) {
return TransactionActivity._enabled;
}
// This happens only in test cases where the integration isn't initalized properly
if (!TransactionActivity.options || isNaN(TransactionActivity.options.tracesSampleRate)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remember that isNaN('') isNaN([]) isNaN(null) isNaN(true) isNaN(false) will also report false. You can wrap the input in parseInt to make it better, or use:

typeof TransactionActivity.options.tracesSampleRate !== 'number'

// Reason being at the time we start the inital transaction we do not have a client bound on the hub yet
// therefore configureScope wouldn't be executed and we would miss setting the transaction
// tslint:disable-next-line: no-unsafe-any
(hub as any).getScope().setSpan(span);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sucky :(

@dashed
Copy link
Member

dashed commented Dec 1, 2019

@HazAT @kamilogorek Can the span status also be part of the trace/span context?

@HazAT HazAT merged commit 645ff53 into master Dec 2, 2019
@HazAT HazAT deleted the apm branch December 2, 2019 15:19
@dashed
Copy link
Member

dashed commented Dec 2, 2019

Damn. So fast.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants